third_party/sqlite_google/src/btree.c - Issue 924: sqlite3 pragma get/set user_version implemented

Delta Between Two Patch Sets: third_party/sqlite_google/src/btree.c

Issue 924: sqlite3 pragma get/set user_version implemented (Closed) SVN Base: http://google-gears.googlecode.com/svn/contrib/dimitri.glazkov/database2/gears/

Left Patch Set: Removed starting a transaction, just report SQLITE_MISUSE now. Created 4 months ago

Right Patch Set: moved code to btree.c, restructured Created 4 months, 2 weeks ago

Use n/p to move between diff chunks; N/P to move between comments. Please Sign in to add in-line comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 /*	1 /*

2 ** 2004 April 6	2 ** 2004 April 6

3 **	3 **

4 ** The author disclaims copyright to this source code. In place of	4 ** The author disclaims copyright to this source code. In place of

5 ** a legal notice, here is a blessing:	5 ** a legal notice, here is a blessing:

6 **	6 **

7 ** May you do good and not evil.	7 ** May you do good and not evil.

8 ** May you find forgiveness for yourself and forgive others.	8 ** May you find forgiveness for yourself and forgive others.

9 ** May you share freely, never taking more than you give.	9 ** May you share freely, never taking more than you give.

10 **	10 **

(...skipping 6411 matching lines...) Show 10 above Show 10 below
6422 return sqlite3BtreeCommit(p);	6422 return sqlite3BtreeCommit(p);

6423	6423

6424 err:	6424 err:

6425 /* TODO(shess): What about errors, here? */	6425 /* TODO(shess): What about errors, here? */

6426 sqlite3BtreeRollback(p);	6426 sqlite3BtreeRollback(p);

6427 return rc;	6427 return rc;

6428 }	6428 }

6429	6429

6430 #endif	6430 #endif

6431	6431

6432 int sqlite3_get_user_version(sqlite3 sqlite, int user_version) {	6432 int sqlite3_get_user_version(sqlite3 sqlite, int user_version) {
Scott.Hess 2008/06/04 17:48:56 Did you copy 'sqlite3 sqlite' from somewhere? If 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. Dimitri* 2008/06/11 01:31:57 On 2008/06/04 17:48:56, Scott.Hess wrote: > Did yo 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.
6433 Btree *bt;	6433 Btree *bt;

6434	6434

6435 if( sqlite == NULL ) return SQLITE_ERROR;	6435 if( sqlite == NULL ) return SQLITE_ERROR;
Scott.Hess 2008/06/04 17:48:56 SQLite style is 'sqlite==NULL' rather than 'sqlite SQLite style is 'sqlite==NULL' rather than 'sqlite == NULL'. Yes, this is the opposite of Gears style :-). Dimitri 2008/06/11 01:31:57 On 2008/06/04 17:48:56, Scott.Hess wrote: > SQLite 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.
6436 if( sqlite->nDb<1 ) return SQLITE_ERROR;	6436 if( sqlite->nDb<1 ) return SQLITE_ERROR;

6437	6437

6438 /* Grab main database. */	6438 /* grab actual (not temporary tables) database */
	Scott.Hess 2008/05/22 23:44:28 Comments should mostly be composed of sentences. Comments should mostly be composed of sentences. Also next comment below. "Temporary" aside is probably confusing, just "Grab main database." should probably suffice. Dimitri 2008/06/04 03:02:16 On 2008/05/22 23:44:28, Scott.Hess wrote: > Commen 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.
6439 bt = sqlite->aDb[0].pBt;	6439 bt = sqlite->aDb[0].pBt;

6440 /* Because sqlite3BtreeGetMeta sets a table read lock, it is safe to use	6440 /* because sqlite3BtreeGetMeta sets a table read lock, it is safe to use

6441 ** without explicit read transaction. Besides, it is very unlikely we will	6441 ** without explicit read transaction. Besides, it is very unlikely we will

6442 ** ever call this outside of a transaction.	6442 ** ever call this outside of a transaction.

6443 */	6443 */
Scott.Hess 2008/06/04 17:48:56 Suggest dropping the caveat about unlikely things. Suggest dropping the caveat about unlikely things. It either needs a lock going in, or it doesn't. Dimitri 2008/06/11 01:31:57 On 2008/06/04 17:48:56, Scott.Hess wrote: > Sugges 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.
6444 return sqlite3BtreeGetMeta(bt, kUserCookie, (u32 *)user_version);	6444 return sqlite3BtreeGetMeta(bt, kUserCookie, (u32 *)user_version);
Scott.Hess 2008/06/04 17:48:56 Why the cast? Alternate phrasing: Why isn't user_ Why the cast? Alternate phrasing: Why isn't user_verion just u32 from the get-go? Dimitri 2008/06/11 01:31:57 On 2008/06/04 17:48:56, Scott.Hess wrote: > Why th 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.
6445 }	6445 }

6446	6446

6447 int sqlite3_set_user_version(sqlite3 *sqlite, int user_version) {	6447 int sqlite3_set_user_version(sqlite3 *sqlite, int user_version) {
Scott.Hess 2008/06/04 17:48:56 Comparable comments to above on sqlite/db and user Comparable comments to above on sqlite/db and user_version/iUserVersion. Dimitri 2008/06/11 01:31:57 On 2008/06/04 17:48:56, Scott.Hess wrote: > Compar On 2008/06/04 17:48:56, Scott.Hess wrote: > Comparable comments to above on sqlite/db and user_version/iUserVersion. Fixed.
6448 Btree *bt;	6448 Btree *bt;

	6449 int rc;

6449	6450

6450 if( sqlite == NULL ) return SQLITE_ERROR;	6451 if( sqlite == NULL ) return SQLITE_ERROR;
Scott.Hess 2008/06/04 17:48:56 No spaces around ==. No spaces around ==. Dimitri 2008/06/11 01:31:57 On 2008/06/04 17:48:56, Scott.Hess wrote: > No spa On 2008/06/04 17:48:56, Scott.Hess wrote: > No spaces around ==. Done.
6451 if( sqlite->nDb<1 ) return SQLITE_ERROR;	6452 if( sqlite->nDb<1 ) return SQLITE_ERROR;

6452	6453

6453 bt = sqlite->aDb[0].pBt;	6454 bt = sqlite->aDb[0].pBt;

6454	6455 if( bt->inTrans!= TRANS_WRITE ) {
	Scott.Hess 2008/05/22 23:44:28 Shouldn't this be ==? No space after = sign in an Shouldn't this be ==? No space after = sign in any case :-). Dimitri 2008/06/04 03:02:16 On 2008/05/22 23:44:28, Scott.Hess wrote: > Should 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.
6455 /* If called outside of a transaction, report misuse. */	6456 return sqlite3BtreeUpdateMeta(bt, kUserCookie, (u32)user_version);

6456 if( bt->inTrans != TRANS_WRITE ) return SQLITE_MISUSE;	6457 }else{
Scott.Hess 2008/06/04 17:48:56 No spaces around !=. No spaces around !=. Dimitri 2008/06/11 01:31:57 On 2008/06/04 17:48:56, Scott.Hess wrote: > No spa On 2008/06/04 17:48:56, Scott.Hess wrote: > No spaces around !=. Done.
6457	6458 /* transacion isn't already in progress, have to enclose in

6458 return sqlite3BtreeUpdateMeta(bt, kUserCookie, (u32)user_version);	6459 ** a write-transaction
Scott.Hess 2008/06/04 17:48:56 Comparable comment as above around cast. Comparable comment as above around cast. Dimitri 2008/06/11 01:31:57 On 2008/06/04 17:48:56, Scott.Hess wrote: > Compar On 2008/06/04 17:48:56, Scott.Hess wrote: > Comparable comment as above around cast. Done.
6459 }	6460 */
	Scott.Hess 2008/05/22 23:44:28 sp on first occurrence of transaction. Revise com sp on first occurrence of transaction. Revise comment to create a sentence.
	6461 rc = sqlite3BtreeBeginTrans(bt, 1);

	6462 if( rc!=SQLITE_OK ) return rc;

	6463 rc = sqlite3BtreeUpdateMeta(bt, kUserCookie, (u32)user_version);

	6464 if( rc==SQLITE_OK ) return sqlite3BtreeCommit(bt);

	6465 sqlite3BtreeRollback(bt);

	6466 return rc;
	Scott.Hess 2008/05/22 23:44:28 For Database2, is it even possible to get to this 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. Dimitri 2008/06/04 03:02:16 On 2008/05/22 23:44:28, Scott.Hess wrote: > For Da 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.
	6467 }

	6468 }
	Scott.Hess 2008/05/22 23:44:28 Sorry to be a pain, I'm trying to recall why we're 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 On 2008/05/22 23:44:28, Scott.Hess wrote: > Sorry 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.
LEFT	RIGHT