Remove a condition from sqlite3WalRead() that is unreachable as of the changes to clear entries out of the wal-index hash tables on transaction or savepoint rollback.

FossilOrigin-Name: 394204735a842b04b677cca20485b1578e475d4c

manifest

@@ -1,5 +1,5 @@
-C Mark\sa\scondition\sin\swal.c\sas\sALWAYS().
+C Remove\sa\scondition\sfrom\ssqlite3WalRead()\sthat\sis\sunreachable\sas\sof\sthe\schanges\sto\sclear\sentries\sout\sof\sthe\swal-index\shash\stables\son\stransaction\sor\ssavepoint\srollback.
-D 2010-06-05T14:42:58
+D 2010-06-05T18:12:24
 F Makefile.arm-wince-mingw32ce-gcc fcd5e9cd67fe88836360bb4f9ef4cb7f8e2fb5a0
 F Makefile.in a5cad1f8f3e021356bfcc6c77dc16f6f1952bbc3
 F Makefile.linux-gcc d53183f4aa6a9192d249731c90dbdffbd2c68654
@@ -224,7 +224,7 @@ F src/vdbeblob.c 5327132a42a91e8b7acfb60b9d2c3b1c5c863e0e
 F src/vdbemem.c 2a82f455f6ca6f78b59fb312f96054c04ae0ead1
 F src/vdbetrace.c 864cef96919323482ebd9986f2132435115e9cc2
 F src/vtab.c a0f8a40274e4261696ef57aa806de2776ab72cda
-F src/wal.c 3a448ad3a563b7b9fe6982e69f56fab327eda196
+F src/wal.c 22a58cdfc167d29353480f1a587d844997120518
 F src/wal.h 4ace25262452d17e7d3ec970c89ee17794004008
 F src/walker.c 3112bb3afe1d85dc52317cb1d752055e9a781f8f
 F src/where.c 75fee9e255b62f773fcadd1d1f25b6f63ac7a356
@@ -763,7 +763,7 @@ F test/vtab_err.test 0d4d8eb4def1d053ac7c5050df3024fd47a3fbd8
 F test/vtab_shared.test 0eff9ce4f19facbe0a3e693f6c14b80711a4222d
 F test/wal.test bfec61450b47cdf09f7d2269f9e9967683b8b0fc
 F test/wal2.test 743d9b86041e57ba986dd7e3891c67725f9e2b2b
-F test/wal3.test 91243bd815e35cfda977df071f95ee2be3cfe236
+F test/wal3.test 1c5e9535e0e307f837e059ec45842e7101796a96
 F test/wal_common.tcl 3e953ae60919281688ea73e4d0aa0e1bc94becd9
 F test/walbak.test e7650a26eb4b8abeca9b145b1af1e63026dde432
 F test/walcksum.test 4efa8fb88c32bed8288ea4385a9cc113a5c8f0bf
@@ -817,7 +817,7 @@ F tool/speedtest2.tcl ee2149167303ba8e95af97873c575c3e0fab58ff
 F tool/speedtest8.c 2902c46588c40b55661e471d7a86e4dd71a18224
 F tool/speedtest8inst1.c 293327bc76823f473684d589a8160bde1f52c14e
 F tool/vdbe-compress.tcl d70ea6d8a19e3571d7ab8c9b75cba86d1173ff0f
-P f9d4ae0e8cc5d32c52eb78799f7959dd236ea9de
+P 3fe0cc784ac586358c08f87fba458dfbb5eec6f2
-R 354467ef49285b1e7f4721d72defda65
+R 2aa1b1d87a9dc475383f9885b8e6d33d
 U dan
-Z 2159f852b96b4a4c0e2c6c207c85dab6
+Z e4cbf876bae12325e294a44d5e590e7b

manifest.uuid

@@ -1 +1 @@
-3fe0cc784ac586358c08f87fba458dfbb5eec6f2
+394204735a842b04b677cca20485b1578e475d4c

src/wal.c

@@ -1746,7 +1746,20 @@ static int walTryBeginRead(Wal *pWal, int *pChanged, int useWal, int cnt){
     */
     rc = walLockShared(pWal, WAL_READ_LOCK(0));
     if( rc==SQLITE_OK ){
-      if( pHdr->mxFrame!=pWal->hdr.mxFrame ){
+      if( memcmp(pHdr, &pWal->hdr, sizeof(WalIndexHdr)) ){
+        /* It is not safe to allow the reader to continue here if frames
+        ** may have been appended to the log before READ_LOCK(0) was obtained.
+        ** When holding READ_LOCK(0), the reader ignores the entire log file,
+        ** which implies that the database file contains a trustworthy
+        ** snapshoT. Since holding READ_LOCK(0) prevents a checkpoint from
+        ** happening, this is usually correct.
+        **
+        ** However, if frames have been appended to the log (or if the log 
+        ** is wrapped and written for that matter) before the READ_LOCK(0)
+        ** is obtained, that is not necessarily true. A checkpointer may
+        ** have started to backfill the appended frames but crashed before
+        ** it finished. Leaving a corrupt image in the database file.
+        */
         walUnlockShared(pWal, WAL_READ_LOCK(0));
         return WAL_RETRY;
       }
@@ -1917,29 +1930,6 @@ int sqlite3WalRead(
   **   (iFrame<=iLast): 
   **     This condition filters out entries that were added to the hash
   **     table after the current read-transaction had started.
-  **
-  **   (iFrame>iRead): 
-  **     This filters out a dangerous class of garbage data. The 
-  **     garbage hash slot may refer to a frame with the correct page 
-  **     number, but not the most recent version of the frame. For
-  **     example, if at the start of the read-transaction the WAL
-  **     contains three copies of the desired page in frames 2, 3 and 4,
-  **     the hash table may contain the following:
-  **
-  **       { ..., 2, 3, 4, 99, 99, ..... }
-  **
-  **     The correct answer is to read data from frame 4. But a 
-  **     dirty-read may potentially cause the hash-table to appear as 
-  **     follows to the reader:
-  **
-  **       { ..., 2, 3, 4, 3, 99, ..... }
-  **
-  **     Without this part of the if(...) clause, the reader might
-  **     incorrectly read data from frame 3 instead of 4. This would be
-  **     an error.
-  **
-  ** It is not actually clear to the developers that such a dirty-read
-  ** can occur. But if it does, it should not cause any problems.
   */
   for(iHash=iLast; iHash>0 && iRead==0; iHash-=HASHTABLE_NPAGE){
     volatile HASHTABLE_DATATYPE *aHash;  /* Pointer to hash table */
@@ -1953,7 +1943,8 @@ int sqlite3WalRead(
     if( mxHash > HASHTABLE_NPAGE )  mxHash = HASHTABLE_NPAGE;
     for(iKey=walHash(pgno); aHash[iKey]; iKey=walNextHash(iKey)){
       u32 iFrame = aHash[iKey] + iZero;
-      if( iFrame<=iLast && aPgno[iFrame]==pgno && iFrame>iRead ){
+      if( iFrame<=iLast && aPgno[iFrame]==pgno ){
+        assert( iFrame>iRead );
         iRead = iFrame;
       }
     }

test/wal3.test

@@ -418,8 +418,151 @@ db close
 T delete
 
 #-------------------------------------------------------------------------
-# When opening a read-transaction on a database 
+# When opening a read-transaction on a database, if the entire log has
+# already been copied to the database file, the reader grabs a special
+# kind of read lock (on aReadMark[0]). This test case tests the outcome
+# of the following:
 #
+#   + The reader discovering that between the time when it determined 
+#     that the log had been completely backfilled and the lock is obtained
+#     that a writer has written to the log. In this case the reader should
+#     acquire a different read-lock (not aReadMark[0]) and read the new
+#     snapshot.
+#
+#   + The attempt to obtain the lock on aReadMark[0] fails with SQLITE_BUSY.
+#     This can happen if a checkpoint is ongoing. In this case also simply
+#     obtain a different read-lock.
+#
+catch {db close}
+testvfs T -default 1
+do_test wal3-6.1.1 {
+  file delete -force test.db test.db-journal test.db wal
+  sqlite3 db test.db
+  execsql { PRAGMA journal_mode = WAL }
+  execsql {
+    CREATE TABLE t1(a, b);
+    INSERT INTO t1 VALUES('o', 't');
+    INSERT INTO t1 VALUES('t', 'f');
+  }
+} {}
+do_test wal3-6.1.2 {
+  sqlite3 db2 test.db
+  sqlite3 db3 test.db
+  execsql { BEGIN ; SELECT * FROM t1 } db3
+} {o t t f}
+do_test wal3-6.1.3 {
+  execsql { PRAGMA wal_checkpoint } db2
+} {}
+
+# At this point the log file has been fully checkpointed. However, 
+# connection [db3] holds a lock that prevents the log from being wrapped.
+# Test case 3.6.1.4 has [db] attempt a read-lock on aReadMark[0]. But
+# as it is obtaining the lock, [db2] appends to the log file.
+#
+T filter xShmLock
+T script lock_callback
+proc lock_callback {method file handle spec} {
+  if {$spec == "3 1 lock shared"} {
+    # This is the callback for [db] to obtain the read lock on aReadMark[0].
+    # Disable future callbacks using [T filter {}] and write to the log
+    # file using [db2]. [db3] is preventing [db2] from wrapping the log
+    # here, so this is an append.
+    T filter {}
+    db2 eval { INSERT INTO t1 VALUES('f', 's') }
+  }
+  return SQLITE_OK
+}
+do_test wal3-6.1.4 {
+  execsql {
+    BEGIN;
+    SELECT * FROM t1;
+  }
+} {o t t f f s}
+
+# [db] should be left holding a read-lock on some slot other than 
+# aReadMark[0]. Test this by demonstrating that the read-lock is preventing
+# the log from being wrapped.
+#
+do_test wal3-6.1.5 {
+  db3 eval COMMIT
+  db2 eval { PRAGMA wal_checkpoint }
+  set sz1 [file size test.db-wal]
+  db2 eval { INSERT INTO t1 VALUES('s', 'e') }
+  set sz2 [file size test.db-wal]
+  expr {$sz2>$sz1}
+} {1}
+
+# Test that if [db2] had not interfered when [db] was trying to grab
+# aReadMark[0], it would have been possible to wrap the log in 3.6.1.5.
+#
+do_test wal3-6.1.6 {
+  execsql { COMMIT }
+  execsql { PRAGMA wal_checkpoint } db2
+  execsql {
+    BEGIN;
+    SELECT * FROM t1;
+  }
+} {o t t f f s s e}
+do_test wal3-6.1.7 {
+  db2 eval { PRAGMA wal_checkpoint }
+  set sz1 [file size test.db-wal]
+  db2 eval { INSERT INTO t1 VALUES('n', 't') }
+  set sz2 [file size test.db-wal]
+  expr {$sz2==$sz1}
+} {1}
+
+db3 close
+db2 close
+db close
+
+do_test wal3-6.2.1 {
+  file delete -force test.db test.db-journal test.db wal
+  sqlite3 db test.db
+  sqlite3 db2 test.db
+  execsql { PRAGMA journal_mode = WAL }
+  execsql {
+    CREATE TABLE t1(a, b);
+    INSERT INTO t1 VALUES('h', 'h');
+    INSERT INTO t1 VALUES('l', 'b');
+  }
+} {}
+
+T filter xShmLock
+T script lock_callback
+proc lock_callback {method file handle spec} {
+  if {$spec == "3 1 unlock exclusive"} {
+    T filter {}
+    set ::R [db2 eval {
+      BEGIN;
+      SELECT * FROM t1;
+    }]
+  }
+}
+do_test wal3-6.2.2 {
+  execsql { PRAGMA wal_checkpoint }
+} {}
+do_test wal3-6.2.3 {
+  set ::R
+} {h h l b}
+do_test wal3-6.2.4 {
+  set sz1 [file size test.db-wal]
+  execsql { INSERT INTO t1 VALUES('b', 'c'); }
+  set sz2 [file size test.db-wal]
+  expr {$sz2 > $sz1}
+} {1}
+do_test wal3-6.2.5 {
+  db2 eval { COMMIT }
+  execsql { PRAGMA wal_checkpoint }
+  set sz1 [file size test.db-wal]
+  execsql { INSERT INTO t1 VALUES('n', 'o'); }
+  set sz2 [file size test.db-wal]
+  expr {$sz2 == $sz1}
+} {1}
+ 
+db2 close
+db close
+T delete
+
 
 finish_test