Make calls to sqlite3BtreeRollbackStmt() no-ops when passed a Btree* handle that does not have an open statement transaction. Ticket #3718. (CVS 6342)

FossilOrigin-Name: a1bb1aef0e06140a2d5d5e4b6c10c73ce95c89e0

manifest

@@ -1,5 +1,5 @@
-C Begin\spurging\sdirty\spages\sfrom\sthe\scache\sonce\s90%\sof\sthe\scache\sis\sdirty\s(insteadof\swaiting\suntil\sit\sis\s100%\sdirty).\sThis\simproves\sperformance\sin\ssome\scircumstances\sby\seffectively\sreserving\s10%\sof\sthe\sconfigured\spage-cache\sfor\sfrequently\sreused\sread-only\spages.\s(CVS\s6341)
-D 2009-03-05T14:59:40
+C Make\scalls\sto\ssqlite3BtreeRollbackStmt()\sno-ops\swhen\spassed\sa\sBtree*\shandle\sthat\sdoes\snot\shave\san\sopen\sstatement\stransaction.\sTicket\s#3718.\s(CVS\s6342)
+D 2009-03-12T14:43:28
 F Makefile.arm-wince-mingw32ce-gcc fcd5e9cd67fe88836360bb4f9ef4cb7f8e2fb5a0
 F Makefile.in d64baddbf55cdf33ff030e14da837324711a4ef7
 F Makefile.linux-gcc d53183f4aa6a9192d249731c90dbdffbd2c68654
@@ -103,7 +103,7 @@ F src/auth.c c8b2ab5c8bad4bd90ed7c294694f48269162c627
 F src/backup.c 2d3f31148d7b086c5c72d9edcd04fc2751b0aa6e
 F src/bitvec.c 44f7059ac1f874d364b34af31b9617e52223ba75
 F src/btmutex.c 341502bc496dc0840dcb00cde65680fb0e85c3ab
-F src/btree.c ec710abc5a71eefba93d2b99330ff2eacd941e08
+F src/btree.c 6e7501d7a207dcc15b099e67231bc8cc86ef7fe9
 F src/btree.h 96a019c9f28da38e79940512d7800e419cd8c702
 F src/btreeInt.h 0a4884e6152d7cae9c741e91b830064c19fd2c05
 F src/build.c 741240c8d6a54201fa8757db1ee6efba71be59a2
@@ -493,7 +493,7 @@ F test/pragma2.test 5364893491b9231dd170e3459bfc2e2342658b47
 F test/printf.test 47e9e5bbec8509023479d54ceb71c9d05a95308a
 F test/progress.test 5b075c3c790c7b2a61419bc199db87aaf48b8301 x
 F test/ptrchng.test ef1aa72d6cf35a2bbd0869a649b744e9d84977fc
-F test/quick.test b7acd5b3df583391979d9f9edf98aa85fc95a3f6
+F test/quick.test d93ab4f1eee87b89fddbe938e2f093ce33e7b46a
 F test/quote.test 215897dbe8de1a6f701265836d6601cc6ed103e6
 F test/randexpr1.tcl 40dec52119ed3a2b8b2a773bce24b63a3a746459
 F test/randexpr1.test 1084050991e9ba22c1c10edd8d84673b501cc25a
@@ -556,8 +556,9 @@ F test/temptrigger.test 03093be9967942623232dfdf2a63b832d4e0e4fa
 F test/tester.tcl 66546f6766029384360b24cacb3896376c5f5f69
 F test/thread001.test 06c45ed9597d478e7bbdc2a8937e1ebea2a20a32
 F test/thread002.test 3c03900f03fd2fe8e2fbb1bbdef7fa8206fdb7ad
-F test/thread003.test 6d360c15afe7f6ef6186801d2cb8407bccbe3aa3
+F test/thread003.test a8bc91af1d9d524148dd84e4d6a196ba17521e08
 F test/thread004.test 9d8ea6a9b0d62d35ad0b967e010d723ed99f614a
+F test/thread005.test 5141b3ee8debc99549f62512265a50be36d1b6a6
 F test/thread1.test 862dd006d189e8b0946935db17399dcac2f8ef91
 F test/thread2.test 91f105374f18a66e73a3254c28fe7c77af69bdea
 F test/thread_common.tcl 047f80288b5e1e86bed181097d67e640f1a54a74
@@ -703,7 +704,7 @@ F tool/speedtest16.c c8a9c793df96db7e4933f0852abb7a03d48f2e81
 F tool/speedtest2.tcl ee2149167303ba8e95af97873c575c3e0fab58ff
 F tool/speedtest8.c 2902c46588c40b55661e471d7a86e4dd71a18224
 F tool/speedtest8inst1.c 293327bc76823f473684d589a8160bde1f52c14e
-P d0b2015f1caf2fc60ec82bd8e760f7b61befa3b4
-R 039bb14c767370b62d0edeb68de61ac0
+P 823fe7f5551e121e211d1ede606a7ce7487ffe0d
+R 0a620faaa60e0f8f26633feb77199541
 U danielk1977
-Z 7a03629d714ccf300df7a819fca83b05
+Z e6301158e9a3d6dc90886b4a779a1ded

manifest.uuid

@@ -1 +1 @@
-823fe7f5551e121e211d1ede606a7ce7487ffe0d
+a1bb1aef0e06140a2d5d5e4b6c10c73ce95c89e0

src/btree.c

@@ -9,7 +9,7 @@
 **    May you share freely, never taking more than you give.
 **
 *************************************************************************
-** $Id: btree.c,v 1.571 2009/03/05 04:20:32 shane Exp $
+** $Id: btree.c,v 1.572 2009/03/12 14:43:28 danielk1977 Exp $
 **
 ** This file implements a external (disk-based) database using BTrees.
 ** See the header comment on "btreeInt.h" for additional information.
@@ -2822,18 +2822,16 @@ int sqlite3BtreeBeginStmt(Btree *p){
 ** subtransaction is active, this is a no-op.
 */
 int sqlite3BtreeCommitStmt(Btree *p){
-  int rc;
+  int rc = SQLITE_OK;
   BtShared *pBt = p->pBt;
   sqlite3BtreeEnter(p);
   pBt->db = p->db;
-  assert( pBt->readOnly==0 );
-  if( pBt->inStmt ){
+  if( p->inTrans==TRANS_WRITE && pBt->inStmt ){
     int iStmtpoint = p->db->nSavepoint;
+    assert( pBt->readOnly==0 );
     rc = sqlite3PagerSavepoint(pBt->pPager, SAVEPOINT_RELEASE, iStmtpoint);
-  }else{
-    rc = SQLITE_OK;
+    pBt->inStmt = 0;
   }
-  pBt->inStmt = 0;
   sqlite3BtreeLeave(p);
   return rc;
 }
@@ -2851,9 +2849,9 @@ int sqlite3BtreeRollbackStmt(Btree *p){
   BtShared *pBt = p->pBt;
   sqlite3BtreeEnter(p);
   pBt->db = p->db;
-  assert( pBt->readOnly==0 );
-  if( pBt->inStmt ){
+  if( p->inTrans==TRANS_WRITE && pBt->inStmt ){
     int iStmtpoint = p->db->nSavepoint;
+    assert( pBt->readOnly==0 );
     rc = sqlite3PagerSavepoint(pBt->pPager, SAVEPOINT_ROLLBACK, iStmtpoint);
     if( rc==SQLITE_OK ){
       rc = sqlite3PagerSavepoint(pBt->pPager, SAVEPOINT_RELEASE, iStmtpoint);

test/quick.test

@@ -6,7 +6,7 @@
 #***********************************************************************
 # This file runs all tests.
 #
-# $Id: quick.test,v 1.93 2009/02/26 07:15:59 danielk1977 Exp $
+# $Id: quick.test,v 1.94 2009/03/12 14:43:28 danielk1977 Exp $
 
 proc lshift {lvar} {
   upvar $lvar l
@@ -86,6 +86,7 @@ set EXCLUDE {
   thread002.test
   thread003.test
   thread004.test
+  thread005.test
   trans2.test
   vacuum3.test
 

test/thread003.test

@@ -12,7 +12,7 @@
 #   This file contains tests that attempt to break the pcache module
 #   by bombarding it with simultaneous requests from multiple threads.
 #     
-# $Id: thread003.test,v 1.6 2009/02/12 17:06:41 drh Exp $
+# $Id: thread003.test,v 1.7 2009/03/12 14:43:28 danielk1977 Exp $
 
 set testdir [file dirname $argv0]
 
@@ -158,7 +158,7 @@ do_test thread003.3 {
 # sqlite3_release_memory() over and over again.
 #
 set nSecond 30
-puts "Starting thread003.3 (should run for ~$nSecond seconds)"
+puts "Starting thread003.4 (should run for ~$nSecond seconds)"
 unset -nocomplain finished(1)
 unset -nocomplain finished(2)
 do_test thread003.4 {

test/thread005.test

@@ -0,0 +1,174 @@
+# 2009 March 11
+#
+# The author disclaims copyright to this source code.  In place of
+# a legal notice, here is a blessing:
+#
+#    May you do good and not evil.
+#    May you find forgiveness for yourself and forgive others.
+#    May you share freely, never taking more than you give.
+#
+#***********************************************************************
+#
+# Test a race-condition that shows up in shared-cache mode.
+#
+# $Id: thread005.test,v 1.1 2009/03/12 14:43:28 danielk1977 Exp $
+
+set testdir [file dirname $argv0]
+
+source $testdir/tester.tcl
+ifcapable !mutex||!shared_cache {
+  return
+}
+source $testdir/thread_common.tcl
+if {[info commands sqlthread] eq ""} {
+  return
+}
+
+#-------------------------------------------------------------------------
+# This test tries to exercise a race-condition that existed in shared-cache
+# mode at one point. The test uses two threads; each has a database connection
+# open on the same shared cache. The schema of the database is:
+#
+#    CREATE TABLE t1(a INTEGER PRIMARY KEY, b UNIQUE);
+#
+# One thread is a reader and the other thread a reader and a writer. The 
+# writer thread repeats the following transaction as fast as possible:
+# 
+#      BEGIN;
+#        DELETE FROM t1 WHERE a = (SELECT max(a) FROM t1);
+#        INSERT INTO t1 VALUES(NULL, NULL);
+#        UPDATE t1 SET b = a WHERE a = (SELECT max(a) FROM t1);
+#        SELECT count(*) FROM t1 WHERE b IS NULL;
+#      COMMIT;
+#
+# The reader thread does the following over and over as fast as possible:
+#
+#      BEGIN;
+#        SELECT count(*) FROM t1 WHERE b IS NULL;
+#      COMMIT;
+#
+# The test runs for 20 seconds or until one of the "SELECT count(*)" 
+# statements returns a non-zero value. If an SQLITE_LOCKED error occurs,
+# the connection issues a ROLLBACK immediately to abandon the current
+# transaction.
+#
+# If everything is working correctly, the "SELECT count(*)" statements 
+# should never return a value other than 0. The "INSERT" statement 
+# executed by the writer adds a row with "b IS NULL" to the table, but
+# the subsequent UPDATE statement sets its "b" value to an integer
+# immediately afterwards.
+#
+# However, before the race-condition was fixed, if the reader's SELECT
+# statement hit an error (say an SQLITE_LOCKED) at the same time as the
+# writer was executing the UPDATE statement, then it could incorrectly
+# rollback the statement-transaction belonging to the UPDATE statement.
+# The UPDATE statement would still be reported as successful to the user,
+# but it would have no effect on the database contents.
+# 
+# Note that it has so far only proved possible to hit this race-condition
+# when using an ATTACHed database. There doesn't seem to be any reason
+# for this, other than that operating on an ATTACHed database means there
+# are a few more mutex grabs and releases during the window of time open
+# for the race-condition. Maybe this encourages the scheduler to context
+# switch or something...
+#
+
+# Use shared-cache mode for this test.
+# 
+db close
+set ::enable_shared_cache [sqlite3_enable_shared_cache]
+sqlite3_enable_shared_cache 1
+
+file delete -force test.db test2.db
+
+do_test thread005-1.1 {
+  sqlite3 db test.db
+  execsql { ATTACH 'test2.db' AS aux }
+  execsql {
+    CREATE TABLE aux.t1(a INTEGER PRIMARY KEY, b UNIQUE);
+    INSERT INTO t1 VALUES(1, 1);
+    INSERT INTO t1 VALUES(2, 2);
+  }
+  db close
+} {}
+
+set ThreadProgram {
+  proc execsql {zSql {db {}}} {
+    if {$db eq ""} {set db $::DB}
+
+    set lRes [list]
+    set rc SQLITE_OK
+
+    while {$rc=="SQLITE_OK" && $zSql ne ""} {
+      set STMT [sqlite3_prepare_v2 $db $zSql -1 zSql]
+      while {[set rc [sqlite3_step $STMT]] eq "SQLITE_ROW"} {
+        for {set i 0} {$i < [sqlite3_column_count $STMT]} {incr i} {
+          lappend lRes [sqlite3_column_text $STMT 0]
+        }
+      }
+      set rc [sqlite3_finalize $STMT]
+    }
+
+    if {$rc != "SQLITE_OK"} { error "$rc [sqlite3_errmsg $db]" }
+    return $lRes
+  }
+
+  if {$isWriter} {
+    set Sql {
+      BEGIN;
+        DELETE FROM t1 WHERE a = (SELECT max(a) FROM t1);
+        INSERT INTO t1 VALUES(NULL, NULL);
+        UPDATE t1 SET b = a WHERE a = (SELECT max(a) FROM t1);
+        SELECT count(*) FROM t1 WHERE b IS NULL;
+      COMMIT;
+    }
+  } else {
+    set Sql {
+      BEGIN;
+      SELECT count(*) FROM t1 WHERE b IS NULL;
+      COMMIT;
+    }
+  }
+
+  set ::DB [sqlite3_open test.db]
+
+  execsql { ATTACH 'test2.db' AS aux }
+
+  set result "ok"
+  set finish [expr [clock_seconds]+5]
+  while {$result eq "ok" && [clock_seconds] < $finish} {
+    set rc [catch {execsql $Sql} msg]
+    if {$rc} {
+      if {[string match "SQLITE_LOCKED*" $msg]} {
+        catch { execsql ROLLBACK }
+      } else {
+        error $msg
+      }
+    } elseif {$msg ne "0"} {
+      set result "failed"
+    }
+  }
+
+  sqlite3_close $::DB
+  set result
+}
+
+puts "Running thread-tests for ~20 seconds"
+thread_spawn finished(0) {set isWriter 0} $ThreadProgram
+thread_spawn finished(1) {set isWriter 1} $ThreadProgram
+if {![info exists finished(0)]} { vwait finished(0) }
+if {![info exists finished(1)]} { vwait finished(1) }
+
+do_test thread005-1.2 {
+  list $finished(0) $finished(1)
+} {ok ok}
+
+do_test thread005-1.3 {
+  sqlite3 db test.db
+  execsql { ATTACH 'test2.db' AS aux }
+  execsql { SELECT count(*) FROM t1 WHERE b IS NULL }
+} {0}
+
+sqlite3_enable_shared_cache $::enable_shared_cache
+finish_test
+