254886 - SQLite3 support for sql module

Status: RESOLVED FIXED

(Core Graveyard :: SQL, enhancement)

Description

[Valia V. Vaneeva]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040616
Build Identifier: 

This bug is for tracking the development of SQLite3 support of the sql module
(http://bugzilla.mozilla.org/show_bug.cgi?id=81653).

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Valia V. Vaneeva]

Attachment: A reworked patch for adding SQLite3 support to the sql module

I hope it's less buggy and more correct.

Now we check file permission to know if we can write to a database.
You can also input not ablsolute paths as databases but relative to your
profile directory.

Comments, improvements, and notices are appreciated.

Comment 2

[Valia V. Vaneeva]

Attachment: XPI for Mac OS X

PI for Mac OS X, provides both PostgreSQL and SQLite support.

Comment 3

[Vidar Haarr (not reading bugmail)]

Has this been checked into the tree? If not, I don't think it should be FIXED -
you should reopen it.

Comment 4

[Valia V. Vaneeva]

Attachment: XPI for Linux

PI for Linux, provides both PostgreSQL and SQLite support.

Comment 5

[Valia V. Vaneeva]

(In reply to comment #3)
> Has this been checked into the tree? If not, I don't think it should be FIXED -
> you should reopen it.
Yes, I was wrong, sorry.

Comment 6

[Valia V. Vaneeva]

Attachment: README

Comment 7

[Vidar Haarr (not reading bugmail)]

Confirming RFE and assigning to developer.
Valia: Thanks for working on this :)

Comment 8

[Valia V. Vaneeva]

(In reply to comment #7)
> Valia: Thanks for working on this :)
%)

Comment 9

[Jan Varga [:janv]]

Comment on attachment 155575 [details] [diff] [review]
A reworked patch for adding SQLite3 support to the sql module

>@@ -1198,7 +1198,7 @@
> 
>   nsCOMPtr<mozISqlResult> result;
>   nsresult rv = mConnection->ExecuteQuery(query, getter_AddRefs(result));
>-
>+  

trailing spaces

> ifdef MOZ_ENABLE_PGSQL
> INCLUDES		+= -I$(MOZ_PGSQL_INCLUDES)
> endif
>+
>+ifdef MOZ_ENABLE_SQLITE
>+INCLUDES        += -I$(MOZ_SQLITE_INCLUDES)
>+endif
>+

wrong alignment

>+ * Contributor(s):
>+ *  Valia Vaneeva <[e-mail]>

two spaces please

>+  nsCOMPtr<nsILocalFile> aFile(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID,
aFile -> file

>+#if defined(XP_WIN) || defined(XP_OS2)
>+  if (*++start == ':') {

what about trying to open aDatabase first and if it fails try to prefix it with
the users profile?
it would be much simplier and more readable

>+NS_IMETHODIMP
>+mozSqlConnectionSqlite::GetPrimaryKeys(const nsAString& aSchema, const nsAString& aTable, mozISqlResult** _retval)
man, this method is really complicated, could you describe what it does?

>+nsresult
>+mozSqlConnectionSqlite::RealExec(const nsAString& aQuery,
>+                                mozISqlResult** aResult, PRInt32* aAffectedRows)
>+{
>+  //sqlite3_changes doesn't reset its count to 0 after selects
>+  static PRInt32 oldChange = 0;
>+  
>+  if (! mConnection)
>+    return NS_ERROR_NOT_INITIALIZED;
>+
>+  char **r, *errmsg;
>+  PRInt32 stat, nrow, ncolumn;
>+
>+  stat = sqlite3_get_table(mConnection, NS_ConvertUCS2toUTF8(aQuery).get(), &r, &nrow, &ncolumn, &errmsg);

hmm, sqlite3_exec with a callback would be more efficient, BuildRows() would
return just NS_OK;
you'll also need to set SHOW_DATATYPES to ON probably.

>+    PRBool				aWritable;
mWritable - it's a Member variable

>diff -urN sql.orig/sqlite/src/mozSqlConnectionSqlite2.cpp sql/sqlite/src/mozSqlConnectionSqlite2.cpp
two similar files?

>+void
>+mozSqlResultSqlite::SetResult(char** aResult, PRInt32 nrow, PRInt32 ncolumn, PRBool aWritable)
>+{
>+  mResult = aResult;
>+  mColumn = ncolumn;
>+  mRow = nrow;
>+  mWritable = aWritable;
>+}

nrow->aRow or aRowCount
ncolumn->aColumn or aColumnCount

>+mozSqlResultSqlite::GetColType(PRInt32 aColumnIndex)
>+{
>+  return mozISqlResult::TYPE_STRING;
>+}

someone will need to fix it in the future - add |XXX - ...|
(I know pgsql implementation isn't correct too)

>+
>+nsresult
>+mozSqlResultSqlite::BuildColumnInfo()
>+{
>+  for (PRInt32 i = 0; i < mColumn; i++) {

mColumn -> mColumnCount ?

>+    char* n = mResult[i];
>+    PRUnichar* name = UTF8ToNewUnicode(nsDependentCString(n));
>+    PRInt32 type = GetColType(i);
>+    PRUint32 max = 1;
>+
>+    for (PRInt32 j = 1; j <= mRow; j++) {

mRow -> mRowCount ?
and I don't think this is right, size isn't the max length in the result
anyway, you don't need to waste too much time with this for now

>+      if (mResult[i+j*mColumn])
>+	{
>+  	  if (max < strlen(mResult[i+j*mColumn]))
>+	    max = strlen(mResult[i+j*mColumn]);
>+	}
>+    }
>+    PRInt32 size = max;
>+    PRInt32 mod = -1;
>+

>+nsresult
>+mozSqlResultSqlite::EnsureTablePrivileges()
>+{
>+  mCanInsert = mWritable;
>+  mCanUpdate = mWritable;
>+  mCanDelete = mWritable;
>+  
>+  return NS_OK;
>+}

I don't think you need this method.

>+
>+nsresult
>+mozSqlResultSqlite::CanInsert(PRBool* _retval)
>+{
>+  if (mCanInsert >= 0) {
>+    *_retval = mCanInsert;
>+    return NS_OK;
>+  }
>+
>+  nsresult rv = EnsureTablePrivileges();
>+  if (NS_FAILED(rv))
>+    return rv;
>+  *_retval = mCanInsert;

I'd change it to |*_retval = mWritable|

Comment 10

[Valia V. Vaneeva]

(In reply to comment #9)
> what about trying to open aDatabase first and if it fails try to prefix it with
> the users profile?
> it would be much simplier and more readable
If we do this way, can it open something which is not intended to be open, just
by chance (if we have a relative path)?

> man, this method is really complicated, could you describe what it does?
Sorry, I'll add comments.

> hmm, sqlite3_exec with a callback would be more efficient, BuildRows() would
> return just NS_OK;
I'll think of it, I'm not yet sure of how to use sqlite3_exec the right way.

> you'll also need to set SHOW_DATATYPES to ON probably.
What does it do? I haven't found anything about it in SQLite docs %(

> two similar files?
This one shouldn't be here, it's my mistake.
 
> someone will need to fix it in the future - add |XXX - ...|
> (I know pgsql implementation isn't correct too)
Why?
I mean there's no limitations what to store where, moreover, a value type is
associated with the value itself not with its column. How to build a column info
then? Or do I miss something?
 
> and I don't think this is right, size isn't the max length in the result
> anyway, you don't need to waste too much time with this for now
But how should I determine column size? Stored strings can be of any length.

Everything else will be fixed.

Comment 11

[Valia V. Vaneeva]

(In reply to comment #10)
> (In reply to comment #9)
> > what about trying to open aDatabase first and if it fails try to prefix it with
> > the users profile?
> > it would be much simplier and more readable
> If we do this way, can it open something which is not intended to be open, just
> by chance (if we have a relative path)?
As far as I see it's impossible. Will be fixed.

Comment 12

[Jan Varga [:janv]]

(In reply to comment #10)
> > hmm, sqlite3_exec with a callback would be more efficient, BuildRows() would
> > return just NS_OK;
> I'll think of it, I'm not yet sure of how to use sqlite3_exec the right way.

I found it by chance in the sqlite documentation, it looks promising, we could
avoid building of two buffers.

> 
> > you'll also need to set SHOW_DATATYPES to ON probably.
> What does it do? I haven't found anything about it in SQLite docs %(
> 

I found it here http://www.sqlite.org/c_interface.html
It might not be needed in the new version (3.0.x).
You just need to know column types in the callback function.

>  
> > someone will need to fix it in the future - add |XXX - ...|
> > (I know pgsql implementation isn't correct too)
> Why?
> I mean there's no limitations what to store where, moreover, a value type is
> associated with the value itself not with its column. How to build a column info
> then? Or do I miss something?

We can't use TYPE_STRING for all columns, because sorting wouldn't work
correctly for integers, dates, etc.

>  
> > and I don't think this is right, size isn't the max length in the result
> > anyway, you don't need to waste too much time with this for now
> But how should I determine column size? Stored strings can be of any length.
> 
> Everything else will be fixed.

yeah, stored strings can be of any length, so the size is undefined (-1)
but as I said, don't waste time with it, consider this part as experimental,
since it doesn't work correctly in the psql implementation too
Anyway, going through all rows and columns is not very efficient.

Comment 13

[Jan Varga [:janv]]

Also, don't spend too much time with sql_exec either. It's just a matter of
performance, so it can be fixed later.

Comment 14

[Valia V. Vaneeva]

(In reply to comment #11)
> As far as I see it's impossible. Will be fixed.
Or possible %) Because we use sqlite3_open and if we have a database with the
same name in the current working dir, it will be opened insted of the one in the
profile.

And we have to check the existence of the file before opening it or new empty
files will be created and that is not good.

Sorry, Jan, I can't work on it right now, only in the evening, so you'll have to
wait a bit %(

Comment 15

[Valia V. Vaneeva]

Attachment: patch for adding SQLite3 support

Comment 16

[Valia V. Vaneeva]

Attachment: XPI for Mac OS X

Comment 17

[Valia V. Vaneeva]

Attachment: XPI for Mac Linux

Comment 18

[Valia V. Vaneeva]

(In reply to comment #12)
> > Why?
> > I mean there's no limitations what to store where, moreover, a value type is
> > associated with the value itself not with its column. How to build a column info
> > then? Or do I miss something?
> We can't use TYPE_STRING for all columns, because sorting wouldn't work
> correctly for integers, dates, etc.
Even if we set the type INTEGER for a column it will still be able to store
strings, not only integers, so you mean you want to sort such strings like integers?

SQLite but itself distinguish column (not values) types using only "create
table" statement and you still can ignore those types. Are you sure we anything
else except strings?

> yeah, stored strings can be of any length, so the size is undefined (-1)
> but as I said, don't waste time with it, consider this part as experimental,
> since it doesn't work correctly in the psql implementation too
> Anyway, going through all rows and columns is not very efficient.
Fixed.

> Also, don't spend too much time with sql_exec either. It's just a matter of
> performance, so it can be fixed later.
Ok. Now I see what you mean but we have to get info about columns first %(

Comment 19

[Jan Varga [:janv]]

(In reply to comment #14)
> (In reply to comment #11)
> > As far as I see it's impossible. Will be fixed.
> Or possible %) Because we use sqlite3_open and if we have a database with the
> same name in the current working dir, it will be opened insted of the one in the
> profile.
> 
> And we have to check the existence of the file before opening it or new empty
> files will be created and that is not good.

Hmm, sounds like there should be an explicit way to store the database in the
profile. We will need to redo alias manager in the future anyway.
For now, it's ok as it is.

Comment 20

[Jan Varga [:janv]]

(In reply to comment #18)
> Even if we set the type INTEGER for a column it will still be able to store
> strings, not only integers, so you mean you want to sort such strings like
integers?

I don't get it. GetColType() should return one of the types defined in 
http://lxr.mozilla.org/mozilla/source/extensions/sql/base/public/mozISqlResult.idl#101
Using TYPE_STRING for all columns is slower/inefficient and not correct because
of sorting.
Anyway it's ok to use strings for start, but it should be fixed in the future.
Each mozISqlResult implementation is slightly different, because each SQL engine
is different. GetColType() should call a database specific API to get column
types, like pgsql do
http://lxr.mozilla.org/mozilla/source/extensions/sql/pgsql/src/mozSqlResultPgsql.cpp#72

> SQLite but itself distinguish column (not values) types using only "create
> table" statement and you still can ignore those types. Are you sure we anything
> else except strings?
> 

ignore those types?
sounds weird to me

> Ok. Now I see what you mean but we have to get info about columns first %(
That's why I mentioned the SHOW_DATATYPES pragma.
Check http://www.sqlite.org/c_interface.html

Comment 21

[Jan Varga [:janv]]

Comment on attachment 155712 [details] [diff] [review]
patch for adding SQLite3 support

>+  nsAutoString aPath;
path

>+  nsCOMPtr<nsILocalFile> aFile(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID,
file

>+  if (rv == NS_ERROR_FILE_UNRECOGNIZED_PATH) {
>+    nsCOMPtr<nsIProperties> directoryService = do_GetService(
>+    										   NS_DIRECTORY_SERVICE_CONTRACTID,
>+    										   &rv);
>+  if (NS_FAILED(rv))
>+    return rv;

wrong alignment

>+mozSqlResultSqlite::CanInsert(PRBool* _retval)
>+{
>+  if (mCanInsert >= 0) {
>+    *_retval = mCanInsert;
>+    return NS_OK;
>+  }

I think, there's no need to check mCanInsert here.
mCanInsert/mCanUpdate/mCanDelete will be probably moved to mozSqlResultPgsql

>+    nsresult EnsureTablePrivileges();
>+    virtual nsresult CanInsert(PRBool* _retval);
no need for EnsureTablePrivileges() in the header either

there are also tabs instead of spaces here and there

Comment 22

[Valia V. Vaneeva]

(In reply to comment #20)
> Each mozISqlResult implementation is slightly different, because each SQL engine
> is different. GetColType() should call a database specific API to get column
> types, like pgsql do
Ok.
 
> ignore those types?
> sounds weird to me
http://www.sqlite.org/datatype3.html

> That's why I mentioned the SHOW_DATATYPES pragma.
> Check http://www.sqlite.org/c_interface.html
It's no more used: http://www.sqlite.org/lang.html#pragma
Using pragmas doesn't sound good for me.

Comment 23

[Valia V. Vaneeva]

(In reply to comment #21)
> path

> file

> wrong alignment

> I think, there's no need to check mCanInsert here.
> mCanInsert/mCanUpdate/mCanDelete will be probably moved to mozSqlResultPgsql

> no need for EnsureTablePrivileges() in the header either

> there are also tabs instead of spaces here and there

Will be fixed.

Comment 24

[Jan Varga [:janv]]

>http://www.sqlite.org/datatype3.html
Ah, now I see.
Anyway, it's not typical for a db engine to behave this way AFAIK.

Comment 25

[Valia V. Vaneeva]

(In reply to comment #24)
> >http://www.sqlite.org/datatype3.html
> Ah, now I see.
> Anyway, it's not typical for a db engine to behave this way AFAIK.
So, what do you think about it? Should I try to determine types?

Comment 26

[Jan Varga [:janv]]

(In reply to comment #25)
> So, what do you think about it? Should I try to determine types?

Definitely, although it can be fixed later in a different bug. There are also
problems with parsing of dates. That's why pgsql implementation returns only
TYPE_STRING and TYPE_BOOL for now.

Comment 27

[Valia V. Vaneeva]

Attachment: patch for adding SQLite3 support

Contains minor changes.

Comment 28

[Valia V. Vaneeva]

(In reply to comment #26)
> Definitely, although it can be fixed later in a different bug. There are also
> problems with parsing of dates. That's why pgsql implementation returns only
> TYPE_STRING and TYPE_BOOL for now.
Oh, SQLite3 doesn't even know about dates %)
Okay, it'll be fixed later, anyway, I need some time to think.

Comment 29

[Jan Varga [:janv]]

Comment on attachment 156048 [details] [diff] [review]
patch for adding SQLite3 support

that's it
r=varga

Comment 30

[[e-mail]]

Sorry to ask this here ... what exactly is the plan for integrating SQLite in
FF/TB? Will it be available as an XPCOM component, so that extensions can use it
for backend storage (e.g. for email meta-data)? 

Comment 31

[Jan Varga [:janv]]

Comment on attachment 156048 [details] [diff] [review]
patch for adding SQLite3 support

checked in, no sr needed since it's not part of the build

Comment 32

[Valia V. Vaneeva]

(In reply to comment #30)
> Sorry to ask this here ... what exactly is the plan for integrating SQLite in
> FF/TB? Will it be available as an XPCOM component, so that extensions can use it
> for backend storage (e.g. for email meta-data)? 
I've tried to make an XPI of the sql module for Firefox but it failed when
trying to fill in the aliases template.

BTW, Jan, do you have a roadmap or any other plan/proposal?

Comment 33

[Grigory Batalov]

(In reply to comment #32)

> > Sorry to ask this here ... what exactly is the plan for integrating SQLite
in FF/TB?

> I've tried to make an XPI of the sql module for Firefox but it failed when
> trying to fill in the aliases template.
> 
> BTW, Jan, do you have a roadmap or any other plan/proposal?

I have built firefox 1.04 for Linux with PgSQL/MySQL/SQLite enabled, but I need
this extension for Windows too. Actually, just SQLite would be enough. Can
anybody build it for me or help with suggestions?

BTW, I have rolled back some of CVS changes to make aliases work.