sqlite3 pragma get/set user_version implemented

Added direct pragma modification calls to sqlite3 API.

[GvR, 2008-05-10 16:15:02]

Dmitri, through scanning the logs for errors, I noticed the base url of your
issue was somehow off, preventing the side-by-side diffs to work. I hope you
don't mind I added the missing 'gears' to the base. This issue can now be
reviewed properly.

[Aaron, 2008-05-11 02:37:57]

LGTM

[Scott.Hess, 2008-05-20 18:37:03]

Sorry for the delay - I'm gradually working my way through all my deferred
gears-related emails ...

http://codereview.appspot.com/924/diff/1/2
File third_party/sqlite_google/preprocessed/sqlite3.h (right):

http://codereview.appspot.com/924/diff/1/2#newcode2689
Line 2689: #endif /* SQLITE_OMIT_PRAGMA || SQLITE_OMIT_PARSER */
Just to make really really sure - this change is from sqlite3.h.in after running
through the instructions in README.google?

http://codereview.appspot.com/924/diff/1/3
File third_party/sqlite_google/src/pragma.c (right):

http://codereview.appspot.com/924/diff/1/3#newcode17
Line 17: #include "btreeInt.h"
Maybe the new code should just live in btree.c?

http://codereview.appspot.com/924/diff/1/3#newcode1136
Line 1136: int sqlite3_pragma_get_user_version(sqlite3 *sqlite, int
*user_version) {
Suggest not naming this with "pragma", since it's not a pragma.  Also, isn't
this inside the !SQLITE_OMIT_PRAGMA #ifdef anyhow?

http://codereview.appspot.com/924/diff/1/3#newcode1137
Line 1137: // grab actual (not temporary tables) database
This ain't c++!  /* ... */, baby!

http://codereview.appspot.com/924/diff/1/3#newcode1138
Line 1138: Btree *bt = sqlite->aDb[0].pBt;
Suggest a NULL check for sqlite, mainly because the poison code (end of btree.c)
does it, so I assume there was a reason.  That code also checks nDb to make sure
that aDb[0] is really there.

http://codereview.appspot.com/924/diff/1/3#newcode1139
Line 1139: // 6 = user_version cookie
Suggest kUserVersionCookie or somesuch, so that you can use the symbolic name in
set_user_version.

http://codereview.appspot.com/924/diff/1/3#newcode1146
Line 1146: int sqlite3_pragma_set_user_version(sqlite3 *sqlite, int
user_version) {
Same checks down here, of course.

http://codereview.appspot.com/924/diff/1/3#newcode1152
Line 1152: if (needs_transaction) {
SQLite code does if( needs_transaction ){.  Indeed, that is exactly opposite of
our conventions!

http://codereview.appspot.com/924/diff/1/3#newcode1155
Line 1155: if (rc != SQLITE_OK) {
SQLite convention would be if( rc!=SQLITE_OK ) return rc;

http://codereview.appspot.com/924/diff/1/3#newcode1167
Line 1167: return rc;
Have you tried a version of this which looked like:

if( bt->inTrans==TRANS_WRITE ){
  return sqlite3BtreeUpdateMeta(...);
}else{
  int rc = sqlite3BTreeBeginTrans(bt, 1);
  if( rc!=SQLITE_OK ) return rc;
  rc = sqlite3BtreeUpdateMeta(...);
  if( rc==SQLITE_OK ) return sqlite3BtreeCommit(bt);
  sqlite3BtreeRollback(bt);
  return rc;
}

This field is too small for me to say if that couldn't be further improved by
some re-ordering.

http://codereview.appspot.com/924/diff/1/4
File third_party/sqlite_google/src/sqlite.h.in (right):

http://codereview.appspot.com/924/diff/1/4#newcode2668
Line 2668: #if !defined(SQLITE_OMIT_PRAGMA) && !defined(SQLITE_OMIT_PARSER)
Shouldn't this be #if defined(SQLITE_OMIT_PRAGMA)?

From 10,000 feet, is there any reason we shouldn't just include this without the
#ifdef?  For our code, we're just going to use this every time (even if we
re-enable PRAGMA, I doubt we'll want to bother setting up a PRAGMA statement and
executing it).

Also, this code should be more clearly marked Gears-specific  I'd probably drop
the CAPI3REF:, too, because it's part of a documentation system which we aren't
going to play nice with.

[Dimitri, 2008-05-21 13:14:30]

Thanks for the comments, Scott! Ready for another look.

http://codereview.appspot.com/924/diff/1/2
File third_party/sqlite_google/preprocessed/sqlite3.h (right):

http://codereview.appspot.com/924/diff/1/2#newcode2689
Line 2689: #endif /* SQLITE_OMIT_PRAGMA || SQLITE_OMIT_PARSER */
On 2008/05/20 18:37:04, Scott.Hess wrote:
> Just to make really really sure - this change is from sqlite3.h.in after
running
> through the instructions in README.google?

Umm... It is now! :) I must admit, it was just a transplant. I actually
attempted to do this the first time around, but being in a Win32 environment,
had problems with UnxUtils crashing while running configure. 

But I downloaded MinGW and was able to get this done right -- obviously had to
comment out the g4 stuff.

http://codereview.appspot.com/924/diff/1/3
File third_party/sqlite_google/src/pragma.c (right):

http://codereview.appspot.com/924/diff/1/3#newcode17
Line 17: #include "btreeInt.h"
On 2008/05/20 18:37:04, Scott.Hess wrote:
> Maybe the new code should just live in btree.c?

Makes sense. Done.

http://codereview.appspot.com/924/diff/1/3#newcode1136
Line 1136: int sqlite3_pragma_get_user_version(sqlite3 *sqlite, int
*user_version) {
On 2008/05/20 18:37:04, Scott.Hess wrote:
> Suggest not naming this with "pragma", since it's not a pragma.  Also, isn't
> this inside the !SQLITE_OMIT_PRAGMA #ifdef anyhow?

Done.

http://codereview.appspot.com/924/diff/1/3#newcode1137
Line 1137: // grab actual (not temporary tables) database
On 2008/05/20 18:37:04, Scott.Hess wrote:
> This ain't c++!  /* ... */, baby!

Done.

http://codereview.appspot.com/924/diff/1/3#newcode1138
Line 1138: Btree *bt = sqlite->aDb[0].pBt;
On 2008/05/20 18:37:04, Scott.Hess wrote:
> Suggest a NULL check for sqlite, mainly because the poison code (end of
btree.c)
> does it, so I assume there was a reason.  That code also checks nDb to make
sure
> that aDb[0] is really there.

Done. The code now returns SQLITE_ERROR (missing database) in both cases.

http://codereview.appspot.com/924/diff/1/3#newcode1139
Line 1139: // 6 = user_version cookie
On 2008/05/20 18:37:04, Scott.Hess wrote:
> Suggest kUserVersionCookie or somesuch, so that you can use the symbolic name
in
> set_user_version.

Done.

http://codereview.appspot.com/924/diff/1/3#newcode1146
Line 1146: int sqlite3_pragma_set_user_version(sqlite3 *sqlite, int
user_version) {
On 2008/05/20 18:37:04, Scott.Hess wrote:
> Same checks down here, of course.

Done.

http://codereview.appspot.com/924/diff/1/3#newcode1152
Line 1152: if (needs_transaction) {
On 2008/05/20 18:37:04, Scott.Hess wrote:
> SQLite code does if( needs_transaction ){.  Indeed, that is exactly opposite
of
> our conventions!

Done.

http://codereview.appspot.com/924/diff/1/3#newcode1155
Line 1155: if (rc != SQLITE_OK) {
On 2008/05/20 18:37:04, Scott.Hess wrote:
> SQLite convention would be if( rc!=SQLITE_OK ) return rc;

Done.

http://codereview.appspot.com/924/diff/1/3#newcode1167
Line 1167: return rc;
On 2008/05/20 18:37:04, Scott.Hess wrote:
> Have you tried a version of this which looked like:
> 
> if( bt->inTrans==TRANS_WRITE ){
>   return sqlite3BtreeUpdateMeta(...);
> }else{
>   int rc = sqlite3BTreeBeginTrans(bt, 1);
>   if( rc!=SQLITE_OK ) return rc;
>   rc = sqlite3BtreeUpdateMeta(...);
>   if( rc==SQLITE_OK ) return sqlite3BtreeCommit(bt);
>   sqlite3BtreeRollback(bt);
>   return rc;
> }
> 
> This field is too small for me to say if that couldn't be further improved by
> some re-ordering.
> 

Restructured as you suggested. Done.

http://codereview.appspot.com/924/diff/1/4
File third_party/sqlite_google/src/sqlite.h.in (right):

http://codereview.appspot.com/924/diff/1/4#newcode2668
Line 2668: #if !defined(SQLITE_OMIT_PRAGMA) && !defined(SQLITE_OMIT_PARSER)
On 2008/05/20 18:37:04, Scott.Hess wrote:
> Shouldn't this be #if defined(SQLITE_OMIT_PRAGMA)?
> 
> From 10,000 feet, is there any reason we shouldn't just include this without
the
> #ifdef?  For our code, we're just going to use this every time (even if we
> re-enable PRAGMA, I doubt we'll want to bother setting up a PRAGMA statement
and
> executing it).

I agree. Removed #ifdefs.

> Also, this code should be more clearly marked Gears-specific  I'd probably
drop
> the CAPI3REF:, too, because it's part of a documentation system which we
aren't
> going to play nice with.

Replaced CAPI3REF: with "Gears-specific:" I think the phrasing makes pretty good
sense in this context, let me know if it doesn't work.

http://codereview.appspot.com/924/diff/1/4#newcode2702
Line 2702: #endif
As an aside, what would be a good place for unit tests for this? Should I just
put them in gears/database2/ dir?

[Scott.Hess, 2008-05-22 23:44:27]

Main thing about using the automated process for generating sqlite3.h is so that
we don't end up with a bunch of minor changes when someone runs it in the
future!  I can very easily run it from here, if mingw or whatever is annoying.

I'd say to put the unittests in with SQLite's tests, but it's hard to see how to
accomplish that.  I'd guess you'd want something parallel to
database/common/database_utils_test.cc at some point?

http://codereview.appspot.com/924/diff/83/141
File third_party/sqlite_google/src/btree.c (right):

http://codereview.appspot.com/924/diff/83/141#newcode6438
Line 6438: /* grab actual (not temporary tables) database */
Comments should mostly be composed of sentences.  Also next comment below. 
"Temporary" aside is probably confusing, just "Grab main database." should
probably suffice.

http://codereview.appspot.com/924/diff/83/141#newcode6455
Line 6455: if( bt->inTrans!= TRANS_WRITE ) {
Shouldn't this be ==?  No space after = sign in any case :-).

http://codereview.appspot.com/924/diff/83/141#newcode6460
Line 6460: */
sp on first occurrence of transaction.  Revise comment to create a sentence.

http://codereview.appspot.com/924/diff/83/141#newcode6466
Line 6466: return rc;
For Database2, is it even possible to get to this point and not be in a
transaction?  Given how we're using this function, I'd almost be willing to
throw SQLITE_MISUSE or something of the sort if a transaction is not open.  I
worry about messing with SQLite's transaction machinery.

http://codereview.appspot.com/924/diff/83/141#newcode6468
Line 6468: }
Sorry to be a pain, I'm trying to recall why we're doing this this way.  Doing
the transaction magic makes me worry that we're going to screw up transactions
in some way.  Did you try writing code up in database_utils.cc to lift off the
authorizer while running PRAGMA user-version?  The authorizer's userdata
parameter should be able to accomplish this (allowing the authorizer to change
results based on state).  There's currently no context which can be associated
with a particular sqlite3 handle, but OpenSqliteDatabase() is always called in
creating a Database object, so such context could reasonably be added.

[Note that this thinking is 90% around the transaction magic.  If we removed
that, then I'd be less worried about doing things this way.]

[Dimitri, 2008-06-04 03:02:16]

Ready for another look. I'll add unit tests after rebranching.

http://codereview.appspot.com/924/diff/83/141
File third_party/sqlite_google/src/btree.c (right):

http://codereview.appspot.com/924/diff/83/141#newcode6438
Line 6438: /* grab actual (not temporary tables) database */
On 2008/05/22 23:44:28, Scott.Hess wrote:
> Comments should mostly be composed of sentences.  Also next comment below. 
> "Temporary" aside is probably confusing, just "Grab main database." should
> probably suffice.

Done.

http://codereview.appspot.com/924/diff/83/141#newcode6455
Line 6455: if( bt->inTrans!= TRANS_WRITE ) {
On 2008/05/22 23:44:28, Scott.Hess wrote:
> Shouldn't this be ==?  No space after = sign in any case :-).

Done. See discussion on this below.

http://codereview.appspot.com/924/diff/83/141#newcode6466
Line 6466: return rc;
On 2008/05/22 23:44:28, Scott.Hess wrote:
> For Database2, is it even possible to get to this point and not be in a
> transaction?  Given how we're using this function, I'd almost be willing to
> throw SQLITE_MISUSE or something of the sort if a transaction is not open.  I
> worry about messing with SQLite's transaction machinery.

I think this is a very good idea. I am going with it. I modified the code to
return SQLITE_MISUSE if called outside of a transaction.

http://codereview.appspot.com/924/diff/83/141#newcode6468
Line 6468: }
On 2008/05/22 23:44:28, Scott.Hess wrote:
> Sorry to be a pain, I'm trying to recall why we're doing this this way.  Doing
> the transaction magic makes me worry that we're going to screw up transactions
> in some way.  Did you try writing code up in database_utils.cc to lift off the
> authorizer while running PRAGMA user-version?  The authorizer's userdata
> parameter should be able to accomplish this (allowing the authorizer to change
> results based on state).  There's currently no context which can be associated
> with a particular sqlite3 handle, but OpenSqliteDatabase() is always called in
> creating a Database object, so such context could reasonably be added.
> 
> [Note that this thinking is 90% around the transaction magic.  If we removed
> that, then I'd be less worried about doing things this way.]

If I recall correctly, I started with OP_SetCookie/OP_ReadCookie and traced back
to see how a pragma operation would be executed using VDBE interpreter. I can
not vouch for a 100% correct traceback, though, so I am perfectly happy with the
solution you offered.

As for unit tests, unless there are any objectsions, I am putting them into
gears/database2/sqlite_user_version_test.cc|h. This should place them next to
the code that uses the features.

[Scott.Hess, 2008-06-04 17:48:55]

Looking good, I'm happier with the condensed
not-mucking-with-transactions-at-all version.  Left with style comments, which
is a little annoying (SQLite style in some areas is almost opposite Gears
style).

http://codereview.appspot.com/924/diff/321/262
File third_party/sqlite_google/src/btree.c (right):

http://codereview.appspot.com/924/diff/321/262#newcode6432
Line 6432: int sqlite3_get_user_version(sqlite3 *sqlite, int *user_version) {
Did you copy 'sqlite3 *sqlite' from somewhere?  If not, suggest using 'sqlite3
*db'.  At this level, db is a squishy concept, but I think that's the form used
in other code.  [Like I said, if you're copying it from elsewhere in the SQLite
codebase, then ignore me.]

I'd usually expect user_version to be piUserVersion in SQLiteland, which I
believe may be somewhere near Hungary.

http://codereview.appspot.com/924/diff/321/262#newcode6435
Line 6435: if( sqlite == NULL ) return SQLITE_ERROR;
SQLite style is 'sqlite==NULL' rather than 'sqlite == NULL'.  Yes, this is the
opposite of Gears style :-).

http://codereview.appspot.com/924/diff/321/262#newcode6443
Line 6443: */
Suggest dropping the caveat about unlikely things.  It either needs a lock going
in, or it doesn't.

http://codereview.appspot.com/924/diff/321/262#newcode6444
Line 6444: return sqlite3BtreeGetMeta(bt, kUserCookie, (u32 *)user_version);
Why the cast?  Alternate phrasing: Why isn't user_verion just u32 from the
get-go?

http://codereview.appspot.com/924/diff/321/262#newcode6447
Line 6447: int sqlite3_set_user_version(sqlite3 *sqlite, int user_version) {
Comparable comments to above on sqlite/db and user_version/iUserVersion.

http://codereview.appspot.com/924/diff/321/262#newcode6450
Line 6450: if( sqlite == NULL ) return SQLITE_ERROR;
No spaces around ==.

http://codereview.appspot.com/924/diff/321/262#newcode6456
Line 6456: if( bt->inTrans != TRANS_WRITE ) return SQLITE_MISUSE;
No spaces around !=.

http://codereview.appspot.com/924/diff/321/262#newcode6458
Line 6458: return sqlite3BtreeUpdateMeta(bt, kUserCookie, (u32)user_version);
Comparable comment as above around cast.

[Dimitri, 2008-06-11 01:31:56]

Prettified. Unfortunately, due to branching, I am unable to put the new code
side-by-side. Instead, it's in it's own little world:
http://codereview.appspot.com/2101

http://codereview.appspot.com/924/diff/321/262
File third_party/sqlite_google/src/btree.c (right):

http://codereview.appspot.com/924/diff/321/262#newcode6432
Line 6432: int sqlite3_get_user_version(sqlite3 *sqlite, int *user_version) {
On 2008/06/04 17:48:56, Scott.Hess wrote:
> Did you copy 'sqlite3 *sqlite' from somewhere?  If not, suggest using 'sqlite3
> *db'.  At this level, db is a squishy concept, but I think that's the form
used
> in other code.  [Like I said, if you're copying it from elsewhere in the
SQLite
> codebase, then ignore me.]

Nope, that was all me. Changed to *db.

> 
> I'd usually expect user_version to be piUserVersion in SQLiteland, which I
> believe may be somewhere near Hungary.

Ah, yes, the Magyar notation, we meet again. Changed to piUserVersion and
iUserVersion, respectivelly.

http://codereview.appspot.com/924/diff/321/262#newcode6435
Line 6435: if( sqlite == NULL ) return SQLITE_ERROR;
On 2008/06/04 17:48:56, Scott.Hess wrote:
> SQLite style is 'sqlite==NULL' rather than 'sqlite == NULL'.  Yes, this is the
> opposite of Gears style :-).

Fixed.

http://codereview.appspot.com/924/diff/321/262#newcode6443
Line 6443: */
On 2008/06/04 17:48:56, Scott.Hess wrote:
> Suggest dropping the caveat about unlikely things.  It either needs a lock
going
> in, or it doesn't.

Done.

http://codereview.appspot.com/924/diff/321/262#newcode6444
Line 6444: return sqlite3BtreeGetMeta(bt, kUserCookie, (u32 *)user_version);
On 2008/06/04 17:48:56, Scott.Hess wrote:
> Why the cast?  Alternate phrasing: Why isn't user_verion just u32 from the
> get-go?
> 

That's easy to fix. I made user version unsigned int.

http://codereview.appspot.com/924/diff/321/262#newcode6447
Line 6447: int sqlite3_set_user_version(sqlite3 *sqlite, int user_version) {
On 2008/06/04 17:48:56, Scott.Hess wrote:
> Comparable comments to above on sqlite/db and user_version/iUserVersion.

Fixed.

http://codereview.appspot.com/924/diff/321/262#newcode6450
Line 6450: if( sqlite == NULL ) return SQLITE_ERROR;
On 2008/06/04 17:48:56, Scott.Hess wrote:
> No spaces around ==.

Done.

http://codereview.appspot.com/924/diff/321/262#newcode6456
Line 6456: if( bt->inTrans != TRANS_WRITE ) return SQLITE_MISUSE;
On 2008/06/04 17:48:56, Scott.Hess wrote:
> No spaces around !=.

Done.

http://codereview.appspot.com/924/diff/321/262#newcode6458
Line 6458: return sqlite3BtreeUpdateMeta(bt, kUserCookie, (u32)user_version);
On 2008/06/04 17:48:56, Scott.Hess wrote:
> Comparable comment as above around cast.

Done.