Have sqlite3_snapshot_open() avoid a race condition by taking a shared CHECKPOINTER lock while checking pInfo->nBackfillAttempted.

FossilOrigin-Name: 8084eae0bc4f6513b1147fb890a6b2813f1c0a09

manifest

@@ -1,5 +1,5 @@
-C Fix\sspacing\stypo\sin\scomment.\s\sNo\schanges\sto\scode.
+C Have\ssqlite3_snapshot_open()\savoid\sa\srace\scondition\sby\staking\sa\sshared\sCHECKPOINTER\slock\swhile\schecking\spInfo->nBackfillAttempted.
-D 2015-12-10T03:16:47.044
+D 2015-12-10T15:45:15.186
 F Makefile.in 28bcd6149e050dff35d4dcfd97e890cd387a499d
 F Makefile.linux-gcc 7bc79876b875010e8c8f9502eb935ca92aa3c434
 F Makefile.msc e8fdca1cb89a1b58b5f4d3a130ea9a3d28cb314d
@@ -415,7 +415,7 @@ F src/vdbesort.c a7ec02da4494c59dfd071126dd3726be5a11459d
 F src/vdbetrace.c 8befe829faff6d9e6f6e4dee5a7d3f85cc85f1a0
 F src/vtab.c 2a8b44aa372c33f6154208e7a7f6c44254549806
 F src/vxworks.h c18586c8edc1bddbc15c004fa16aeb1e1342b4fb
-F src/wal.c 964af61902acead7de6e95035d0ce597f7019da9
+F src/wal.c 32ee7dc4b689321d1650fba2e937ddc3bccfb06f
 F src/wal.h 907943dfdef10b583e81906679a347e0ec6f1b1b
 F src/walker.c 2e14d17f592d176b6dc879c33fbdec4fbccaa2ba
 F src/where.c b18edbb9e5afabb77f4f27550c471c5c824e0fe7
@@ -1020,7 +1020,7 @@ F test/skipscan2.test d1d1450952b7275f0b0a3a981f0230532743951a
 F test/skipscan3.test ec5bab3f81c7038b43450e7b3062e04a198bdbb5
 F test/skipscan5.test 67817a4b6857c47e0e33ba3e506da6f23ef68de2
 F test/skipscan6.test 5866039d03a56f5bd0b3d172a012074a1d90a15b
-F test/snapshot.test 800e0be4488acb88dd38ff9e9b83edb71d9d5a9d
+F test/snapshot.test f91d907460e7acc01d531834d068e1215ccac7e4
 F test/soak.test 0b5b6375c9f4110c828070b826b3b4b0bb65cd5f
 F test/softheap1.test 843cd84db9891b2d01b9ab64cef3e9020f98d087
 F test/sort.test 3f492e5b7be1d3f756728d2ff6edf4f6091e84cb
@@ -1409,7 +1409,7 @@ F tool/vdbe_profile.tcl 246d0da094856d72d2c12efec03250d71639d19f
 F tool/warnings-clang.sh f6aa929dc20ef1f856af04a730772f59283631d4
 F tool/warnings.sh 48bd54594752d5be3337f12c72f28d2080cb630b
 F tool/win/sqlite.vsix deb315d026cc8400325c5863eef847784a219a2f
-P cb68e9d0738fc7db7316947b4d2aab91aae819f2
+P 3a18526fc2253658dad84c5e600481c8a62efe40
-R 76ea636367cbd8afb53a2670ba03ae31
+R 7bb35c02cb12ea2ff304c777229238a5
-U mistachkin
+U dan
-Z 1d7611d28a33701f209bc754328c05f3
+Z 0b9f17bd41106b46c64f60d98c85b1e3

manifest.uuid

@@ -1 +1 @@
-3a18526fc2253658dad84c5e600481c8a62efe40
+8084eae0bc4f6513b1147fb890a6b2813f1c0a09

src/wal.c

@@ -1217,7 +1217,7 @@ finished:
     */
     pInfo = walCkptInfo(pWal);
     pInfo->nBackfill = 0;
-    pInfo->nBackfillAttempted = 0;
+    pInfo->nBackfillAttempted = pWal->hdr.mxFrame;
     pInfo->aReadMark[0] = 0;
     for(i=1; i<WAL_NREADER; i++) pInfo->aReadMark[i] = READMARK_NOT_USED;
     if( pWal->hdr.mxFrame ) pInfo->aReadMark[1] = pWal->hdr.mxFrame;
@@ -1757,7 +1757,6 @@ static int walCheckpoint(
     ** cannot be backfilled from the WAL.
     */
     mxSafeFrame = pWal->hdr.mxFrame;
-    pInfo->nBackfillAttempted = mxSafeFrame;
     mxPage = pWal->hdr.nPage;
     for(i=1; i<WAL_NREADER; i++){
       /* Thread-sanitizer reports that the following is an unsafe read,
@@ -1790,6 +1789,8 @@ static int walCheckpoint(
       i64 nSize;                    /* Current size of database file */
       u32 nBackfill = pInfo->nBackfill;
 
+      pInfo->nBackfillAttempted = mxSafeFrame;
+
       /* Sync the WAL to disk */
       if( sync_flags ){
         rc = sqlite3OsSync(pWal->pWalFd, sync_flags);
@@ -2296,9 +2297,6 @@ static int walTryBeginRead(Wal *pWal, int *pChanged, int useWal, int cnt){
   }
   if( (pWal->readOnly & WAL_SHM_RDONLY)==0
    && (mxReadMark<mxFrame || mxI==0)
-#ifdef SQLITE_ENABLE_SNAPSHOT
-   && pWal->pSnapshot==0
-#endif
   ){
     for(i=1; i<WAL_NREADER; i++){
       rc = walLockExclusive(pWal, WAL_READ_LOCK(i), 1);
@@ -2313,9 +2311,6 @@ static int walTryBeginRead(Wal *pWal, int *pChanged, int useWal, int cnt){
     }
   }
   if( mxI==0 ){
-#ifdef SQLITE_ENABLE_SNAPSHOT
-    if( pWal->pSnapshot ) return SQLITE_BUSY_SNAPSHOT;
-#endif
     assert( rc==SQLITE_BUSY || (pWal->readOnly & WAL_SHM_RDONLY)!=0 );
     return rc==SQLITE_BUSY ? WAL_RETRY : SQLITE_READONLY_CANTLOCK;
   }
@@ -2410,9 +2405,10 @@ int sqlite3WalBeginReadTransaction(Wal *pWal, int *pChanged){
   if( rc==SQLITE_OK ){
     if( pSnapshot && memcmp(pSnapshot, &pWal->hdr, sizeof(WalIndexHdr))!=0 ){
       /* At this point the client has a lock on an aReadMark[] slot holding
-      ** a value equal to or smaller than pSnapshot->mxFrame.  Verify that
+      ** a value equal to or smaller than pSnapshot->mxFrame, but pWal->hdr
-      ** pSnapshot is still valid before continuing.  Reasons why pSnapshot
+      ** is populated with the wal-index header corresponding to the head
-      ** might no longer be valid:
+      ** of the wal file. Verify that pSnapshot is still valid before
+      ** continuing.  Reasons why pSnapshot might no longer be valid:
       **
       **    (1)  The WAL file has been reset since the snapshot was taken.
       **         In this case, the salt will have changed.
@@ -2426,10 +2422,22 @@ int sqlite3WalBeginReadTransaction(Wal *pWal, int *pChanged){
       assert( pWal->readLock>0 );
       assert( pInfo->aReadMark[pWal->readLock]<=pSnapshot->mxFrame );
 
-      /* Check that the wal file has not been wrapped. Assuming it has not,
+      /* It is possible that there is a checkpointer thread running 
-      ** overwrite pWal->hdr with *pSnapshot and set *pChanged as appropriate
+      ** concurrent with this code. If this is the case, it may be that the
-      ** for opening the snapshot. Or, if the wal file has been wrapped
+      ** checkpointer has already determined that it will checkpoint 
-      ** since pSnapshot was written, return SQLITE_BUSY_SNAPSHOT. */
+      ** snapshot X, where X is later in the wal file than pSnapshot, but 
+      ** has not yet set the pInfo->nBackfillAttempted variable to indicate 
+      ** its intent. To avoid the race condition this leads to, ensure that
+      ** there is no checkpointer process by taking a shared CKPT lock 
+      ** before checking pInfo->nBackfillAttempted.  */
+      rc = walLockShared(pWal, WAL_CKPT_LOCK);
+
+      /* Check that the wal file has not been wrapped. Assuming that it has
+      ** not, also check that no checkpointer has attempted to checkpoint
+      ** any frames beyond pSnapshot->mxFrame. If either of these conditions
+      ** are true, return SQLTIE_BUSY_SNAPSHOT. Otherwise, overwrite pWal->hdr
+      ** with *pSnapshot and set *pChanged as appropriate for opening the
+      ** snapshot.  */
       if( memcmp(pSnapshot->aSalt, pWal->hdr.aSalt, sizeof(pWal->hdr.aSalt))==0
        && pSnapshot->mxFrame>=pInfo->nBackfillAttempted
       ){
@@ -2439,6 +2447,9 @@ int sqlite3WalBeginReadTransaction(Wal *pWal, int *pChanged){
         rc = SQLITE_BUSY_SNAPSHOT;
       }
 
+      /* Release the shared CKPT lock obtained above. */
+      walUnlockShared(pWal, WAL_CKPT_LOCK);
+
       if( rc!=SQLITE_OK ){
         sqlite3WalEndReadTransaction(pWal);
       }

test/snapshot.test

@@ -57,8 +57,6 @@ do_execsql_test 1.3.2 COMMIT
 # Check that a simple case works. Reuse the database created by the
 # block of tests above.
 #
-# UPDATE: This case (2.1) no longer works. 2.2 does.
-#
 do_execsql_test 2.1.0 {
   BEGIN;
     SELECT * FROM t1;
@@ -75,8 +73,11 @@ do_test 2.1.1 {
 
 do_test 2.1.2 {
   execsql BEGIN
-  list [catch { sqlite3_snapshot_open db main $snapshot } msg] $msg
+  sqlite3_snapshot_open db main $snapshot
-} {1 SQLITE_BUSY_SNAPSHOT}
+  execsql {
+    SELECT * FROM t1;
+  }
+} {1 2 3 4 5 6 7 8}
 
 do_test 2.1.3 {
   sqlite3_snapshot_free $snapshot
@@ -99,7 +100,7 @@ do_test 2.2.1 {
   }
 } {1 2 3 4 5 6 7 8 9 10 11 12}
 
-do_test 2.1.2 {
+do_test 2.2.2 {
   execsql BEGIN
   sqlite3_snapshot_open db main $snapshot
   execsql {
@@ -107,7 +108,7 @@ do_test 2.1.2 {
   }
 } {1 2 3 4 5 6 7 8 9 10}
 
-do_test 2.1.3 {
+do_test 2.2.3 {
   sqlite3_snapshot_free $snapshot
   execsql COMMIT
   execsql COMMIT db2