From: dan Date: Wed, 29 Jul 2015 12:14:28 +0000 (+0000) Subject: Only allow UNLOCKED transactions to commit if none of the pages read by the transacti... X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=773d2d6c097980a4b4c9c0a12b0e0c57f893b330;p=thirdparty%2Fsqlite.git Only allow UNLOCKED transactions to commit if none of the pages read by the transaction have been modified since it was opened. FossilOrigin-Name: 0b9718426e44df092850c5d095ce1b84a1e567cf --- diff --git a/manifest b/manifest index c4c044ae5c..2f36ddd5b8 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Add\ssome\stest\scases\sand\sfix\ssome\ssmall\sproblems\swith\sBEGIN\sUNLOCKED\stransactions. -D 2015-07-28T16:46:49.291 +C Only\sallow\sUNLOCKED\stransactions\sto\scommit\sif\snone\sof\sthe\spages\sread\sby\sthe\stransaction\shave\sbeen\smodified\ssince\sit\swas\sopened. +D 2015-07-29T12:14:28.276 F Makefile.arm-wince-mingw32ce-gcc d6df77f1f48d690bd73162294bbba7f59507c72f F Makefile.in 4de3ef40c8b3b75c0c55ff4242a43c8ce1ad90ee F Makefile.linux-gcc 91d710bdc4998cb015f39edf3cb314ec4f4d7e23 @@ -270,8 +270,8 @@ F src/auth.c b56c78ebe40a2110fd361379f7e8162d23f92240 F src/backup.c 4d9134dc988a87838c06056c89c0e8c4700a0452 F src/bitvec.c d1f21d7d91690747881f03940584f4cc548c9d3d F src/btmutex.c 45a968cc85afed9b5e6cf55bf1f42f8d18107f79 -F src/btree.c b184ce6d483dfadc53126a4b7994633ec0d5ef5f -F src/btree.h 550e8447684013a69196ec664142f67538da8ef1 +F src/btree.c 5087b0a1358abd1449165dd14d49962bc3ad9f44 +F src/btree.h 40bd41ef0b71d6f7502725dc159fa0d6bebd8bb3 F src/btreeInt.h 2ad754dd4528baa8d0946a593cc373b890bf859e F src/build.c 28c15c43eefc0066ff64040526ff649c32fe5523 F src/callback.c 7b44ce59674338ad48b0e84e7b72f935ea4f68b0 @@ -315,8 +315,8 @@ F src/os_setup.h c9d4553b5aaa6f73391448b265b89bed0b890faa F src/os_unix.c 388c023582b17890f10c980b30ec1922b471753b F src/os_win.c 40b3af7a47eb1107d0d69e592bec345a3b7b798a F src/os_win.h eb7a47aa17b26b77eb97e4823f20a00b8bda12ca -F src/pager.c c9f22eb831851220c1095863c5797a88f71bea8a -F src/pager.h b6cf6e9552c5b1e7d6d0d8cd7ca51be7802bec72 +F src/pager.c 18aa67541bd2c7d7963f180669ea4e48cd5246f4 +F src/pager.h 41938212a5d80a7068883bb29ea6ac0d4ef3a6d8 F src/parse.y 199145cd982587d70fa3db2f9b11f7ab118f0acd F src/pcache.c cde06aa50962595e412d497e22fd2e07878ba1f0 F src/pcache.h 9968603796240cdf83da7e7bef76edf90619cea9 @@ -395,15 +395,15 @@ F src/vdbe.c fd6832408d9621ad347a65b01a05380fe766232f F src/vdbe.h 7a75045d879118b9d3af7e8b3c108f2f27c51473 F src/vdbeInt.h 8b54e01ad0463590e7cffabce0bc36da9ee4f816 F src/vdbeapi.c adabbd66eb2e3a10f3998485ee0be7e326d06ee4 -F src/vdbeaux.c 27feae106f3f4db298e65d37a9da4962aaff724b +F src/vdbeaux.c 24a03ccbadc8d4cfecf25b6399db95851b10dd21 F src/vdbeblob.c 4f2e8e075d238392df98c5e03a64342465b03f90 F src/vdbemem.c ae38a0d35ae71cf604381a887c170466ba518090 F src/vdbesort.c f5009e7a35e3065635d8918b9a31f498a499976b F src/vdbetrace.c 8befe829faff6d9e6f6e4dee5a7d3f85cc85f1a0 F src/vtab.c 082b35a25a26e3d36f365ca8cd73c1922532f05e F src/vxworks.h c18586c8edc1bddbc15c004fa16aeb1e1342b4fb -F src/wal.c a2e35f042103519ce22050acaf167960446963c7 -F src/wal.h 5188cd85398dc612ae5ed2ed02d193377218be56 +F src/wal.c 4497b466fee8ed56c31672bb35741dd0321ec999 +F src/wal.h cd7ea52439d56571d549b7d4eacdeed58d841385 F src/walker.c c253b95b4ee44b21c406e2a1052636c31ea27804 F src/where.c 909eba3b6db984eb2adfbca9de2c237ee7056adb F src/whereInt.h 5f87e3c4b0551747d119730dfebddd3c54f04047 @@ -1215,7 +1215,7 @@ F test/types3.test 99e009491a54f4dc02c06bdbc0c5eea56ae3e25a F test/unique.test 93f8b2ef5ea51b9495f8d6493429b1fd0f465264 F test/unique2.test 41e7f83c6827605991160a31380148a9fc5f1339 F test/unixexcl.test cd6c765f75e50e8e2c2ba763149e5d340ea19825 -F test/unlocked.test 854f3f428bb2e0ae91cb2400596a8f1ab7eaa409 +F test/unlocked.test 2da6645c3532985715d17dfcb240ef5f09c2e9d4 F test/unordered.test ca7adce0419e4ca0c50f039885e76ed2c531eda8 F test/update.test 6c68446b8a0a33d522a7c72b320934596a2d7d32 F test/uri.test 23662b7b61958b0f0e47082de7d06341ccf85d5b @@ -1367,7 +1367,7 @@ F tool/vdbe_profile.tcl 67746953071a9f8f2f668b73fe899074e2c6d8c1 F tool/warnings-clang.sh f6aa929dc20ef1f856af04a730772f59283631d4 F tool/warnings.sh 48bd54594752d5be3337f12c72f28d2080cb630b F tool/win/sqlite.vsix deb315d026cc8400325c5863eef847784a219a2f -P 8079421604dbd40d03471dad6d12115119b554c2 -R 52e6230fe0fe2d8cb4434e54d864e964 +P 6da0e962ad2aa5e52c1f1b5c3dbf77a2cb16ac2d +R 74dcd93afc2d7e4cc222b666276012ce U dan -Z 849da1cda4069d7075ecc7190a9058c5 +Z 3bd2cb4e2cf9d30cb919b0a0d11717fa diff --git a/manifest.uuid b/manifest.uuid index 23abb2a199..f6215ad409 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -6da0e962ad2aa5e52c1f1b5c3dbf77a2cb16ac2d \ No newline at end of file +0b9718426e44df092850c5d095ce1b84a1e567cf \ No newline at end of file diff --git a/src/btree.c b/src/btree.c index c83a01ad4c..69d4873d0d 100644 --- a/src/btree.c +++ b/src/btree.c @@ -9590,15 +9590,3 @@ int sqlite3BtreeIsReadonly(Btree *p){ ** Return the size of the header added to each page by this module. */ int sqlite3HeaderSizeBtree(void){ return ROUND8(sizeof(MemPage)); } - -int sqlite3BtreeExclusiveLock(Btree *p){ - int rc; - BtShared *pBt = p->pBt; - sqlite3BtreeEnter(p); - rc = sqlite3PagerExclusiveLock(pBt->pPager, pBt->pPage1->pDbPage); - sqlite3BtreeLeave(p); - return rc; -} - - - diff --git a/src/btree.h b/src/btree.h index ae9b2c4e59..50dc8ca290 100644 --- a/src/btree.h +++ b/src/btree.h @@ -270,6 +270,4 @@ void sqlite3BtreeCursorList(Btree*); # define sqlite3SchemaMutexHeld(X,Y,Z) 1 #endif -int sqlite3BtreeExclusiveLock(Btree*); - #endif /* _BTREE_H_ */ diff --git a/src/pager.c b/src/pager.c index 2ea3b90a96..5107edda36 100644 --- a/src/pager.c +++ b/src/pager.c @@ -657,6 +657,7 @@ struct Pager { u32 cksumInit; /* Quasi-random value added to every checksum */ u32 nSubRec; /* Number of records written to sub-journal */ Bitvec *pInJournal; /* One bit for each page in the database file */ + Bitvec *pAllRead; /* Pages read within current UNLOCKED trans. */ sqlite3_file *fd; /* File descriptor for database */ sqlite3_file *jfd; /* File descriptor for main journal */ sqlite3_file *sjfd; /* File descriptor for sub-journal */ @@ -1738,6 +1739,16 @@ static int addToSavepointBitvecs(Pager *pPager, Pgno pgno){ return rc; } +/* +** Free the Pager.pInJournal and Pager.pAllRead bitvec objects. +*/ +static void pagerFreeBitvecs(Pager *pPager){ + sqlite3BitvecDestroy(pPager->pInJournal); + sqlite3BitvecDestroy(pPager->pAllRead); + pPager->pInJournal = 0; + pPager->pAllRead = 0; +} + /* ** This function is a no-op if the pager is in exclusive mode and not ** in the ERROR state. Otherwise, it switches the pager to PAGER_OPEN @@ -1762,8 +1773,7 @@ static void pager_unlock(Pager *pPager){ || pPager->eState==PAGER_ERROR ); - sqlite3BitvecDestroy(pPager->pInJournal); - pPager->pInJournal = 0; + pagerFreeBitvecs(pPager); releaseAllSavepoints(pPager); if( pagerUseWal(pPager) ){ @@ -1999,8 +2009,7 @@ static int pager_end_transaction(Pager *pPager, int hasMaster, int bCommit){ } #endif - sqlite3BitvecDestroy(pPager->pInJournal); - pPager->pInJournal = 0; + pagerFreeBitvecs(pPager); pPager->nRec = 0; sqlite3PcacheCleanAll(pPager->pPCache); sqlite3PcacheTruncate(pPager->pPCache, pPager->dbSize); @@ -4082,7 +4091,7 @@ static int syncJournal(Pager *pPager, int newHdr){ assert( assert_pager_state(pPager) ); assert( !pagerUseWal(pPager) ); - rc = sqlite3PagerExclusiveLock(pPager, 0); + rc = sqlite3PagerExclusiveLock(pPager); if( rc!=SQLITE_OK ) return rc; if( !pPager->noSync ){ @@ -4432,7 +4441,7 @@ static int pagerStress(void *p, PgHdr *pPg){ if( pagerUseWal(pPager) ){ /* If the transaction is a "BEGIN UNLOCKED" transaction, the page ** cannot be flushed to disk. Return early in this case. */ - if( sqlite3WalIsInTrans(pPager->pWal)==0 ) return SQLITE_OK; + if( pPager->pAllRead ) return SQLITE_OK; /* Write a single frame for this page to the log. */ rc = subjournalPageIfRequired(pPg); @@ -5274,6 +5283,14 @@ int sqlite3PagerAcquire( } pPager->hasBeenUsed = 1; + /* If this is an UNLOCKED transaction and the page being read was + ** present in the database file when the transaction was opened, + ** mark it as read in the pAllRead vector. */ + if( pPager->pAllRead && pgno<=pPager->dbOrigSize ){ + rc = sqlite3BitvecSet(pPager->pAllRead, pgno); + if( rc!=SQLITE_OK ) goto pager_acquire_err; + } + /* If the pager is in the error state, return an error immediately. ** Otherwise, request the page from the PCache layer. */ if( pPager->errCode!=SQLITE_OK ){ @@ -5578,6 +5595,7 @@ int sqlite3PagerBegin(Pager *pPager, int exFlag, int subjInMemory){ if( ALWAYS(pPager->eState==PAGER_READER) ){ assert( pPager->pInJournal==0 ); + assert( pPager->pAllRead==0 ); if( pagerUseWal(pPager) ){ /* If the pager is configured to use locking_mode=exclusive, and an @@ -5598,6 +5616,13 @@ int sqlite3PagerBegin(Pager *pPager, int exFlag, int subjInMemory){ */ if( exFlag>=0 ){ rc = sqlite3WalBeginWriteTransaction(pPager->pWal); + }else{ + pPager->pAllRead = sqlite3BitvecCreate(pPager->dbSize); + if( pPager->pAllRead==0 ){ + rc = SQLITE_NOMEM; + }else{ + rc = sqlite3BitvecSet(pPager->pAllRead, 1); + } } }else{ /* Obtain a RESERVED lock on the database file. If the exFlag parameter @@ -6066,7 +6091,7 @@ int sqlite3PagerSync(Pager *pPager, const char *zMaster){ ** Otherwise, either SQLITE_BUSY or an SQLITE_IOERR_XXX error code is ** returned. */ -int sqlite3PagerExclusiveLock(Pager *pPager, PgHdr *pPage1){ +int sqlite3PagerExclusiveLock(Pager *pPager){ int rc = SQLITE_OK; assert( pPager->eState==PAGER_WRITER_CACHEMOD || pPager->eState==PAGER_WRITER_DBMOD @@ -6076,21 +6101,14 @@ int sqlite3PagerExclusiveLock(Pager *pPager, PgHdr *pPage1){ if( 0==pagerUseWal(pPager) ){ rc = pager_wait_on_lock(pPager, EXCLUSIVE_LOCK); }else{ - Wal *pWal = pPager->pWal; - if( 0==sqlite3WalIsInTrans(pWal) ){ - /* TODO: There must be an optimization opportunity here, as this call - ** to PcacheDirtyList() sorts the list of dirty pages, even though it - ** is not really required - and will be sorted again in CommitPhaseOne() - ** in any case. */ - PgHdr *pList = sqlite3PcacheDirtyList(pPager->pPCache); - + if( pPager->pAllRead ){ /* This is an UNLOCKED transaction. Attempt to lock the wal database ** here. If SQLITE_BUSY (but not SQLITE_BUSY_SNAPSHOT) is returned, ** invoke the busy-handler and try again for as long as it returns ** non-zero. */ do { /* rc = sqlite3WalBeginWriteTransaction(pWal); */ - rc = sqlite3WalLockForCommit(pWal, pList, pPage1); + rc = sqlite3WalLockForCommit(pPager->pWal, pPager->pAllRead); }while( rc==SQLITE_BUSY && pPager->xBusyHandler(pPager->pBusyHandlerArg) ); diff --git a/src/pager.h b/src/pager.h index 57d2812701..3c0b9a1a71 100644 --- a/src/pager.h +++ b/src/pager.h @@ -150,7 +150,7 @@ void *sqlite3PagerGetExtra(DbPage *); void sqlite3PagerPagecount(Pager*, int*); int sqlite3PagerBegin(Pager*, int exFlag, int); int sqlite3PagerCommitPhaseOne(Pager*,const char *zMaster, int); -int sqlite3PagerExclusiveLock(Pager*, DbPage *pPage1); +int sqlite3PagerExclusiveLock(Pager*); int sqlite3PagerSync(Pager *pPager, const char *zMaster); int sqlite3PagerCommitPhaseTwo(Pager*); int sqlite3PagerRollback(Pager*); diff --git a/src/vdbeaux.c b/src/vdbeaux.c index f5f9d0fbdd..93a7187ab9 100644 --- a/src/vdbeaux.c +++ b/src/vdbeaux.c @@ -2020,7 +2020,9 @@ static int vdbeCommit(sqlite3 *db, Vdbe *p){ if( sqlite3BtreeIsInTrans(pBt) ){ needXcommit = 1; if( i!=1 ) nTrans++; - rc = sqlite3BtreeExclusiveLock(pBt); + sqlite3BtreeEnter(pBt); + rc = sqlite3PagerExclusiveLock(sqlite3BtreePager(pBt)); + sqlite3BtreeLeave(pBt); } } diff --git a/src/wal.c b/src/wal.c index be018ebc99..5fbdae6a1a 100644 --- a/src/wal.c +++ b/src/wal.c @@ -2573,7 +2573,7 @@ int sqlite3WalBeginWriteTransaction(Wal *pWal){ ** transaction. It may be assumed that no frames have been written to ** the wal file. */ -int sqlite3WalLockForCommit(Wal *pWal, PgHdr *pList, PgHdr *pPage1){ +int sqlite3WalLockForCommit(Wal *pWal, Bitvec *pAllRead){ int rc = walWriteLock(pWal); /* If the database has been modified since this transaction was started, @@ -2590,49 +2590,35 @@ int sqlite3WalLockForCommit(Wal *pWal, PgHdr *pList, PgHdr *pPage1){ volatile WalIndexHdr *pHead; /* Head of the wal file */ pHead = walIndexHdr(pWal); if( memcmp(&pWal->hdr, (void*)pHead, sizeof(WalIndexHdr))!=0 ){ - int bSeenPage1 = 0; /* True if page 1 is in list pList */ - /* TODO: Is this safe? Because it holds the WRITER lock this thread - ** has exclusive access to the live header, but might it be corrupt? */ - PgHdr *pPg; - u32 iLast = pHead->mxFrame; - for(pPg=pList; rc==SQLITE_OK && pPg; pPg=pPg->pDirty){ - u32 iSlot = 0; - rc = walFindFrame(pWal, pPg->pgno, iLast, &iSlot); - if( iSlot>pWal->hdr.mxFrame ){ - sqlite3_log(SQLITE_OK, - "cannot commit UNLOCKED transaction (conflict at page %d)", - (int)pPg->pgno - ); - rc = SQLITE_BUSY_SNAPSHOT; - } - if( pPg->pgno==1 ) bSeenPage1 = 1; - } - - /* If the current transaction does not modify page 1 of the database, - ** check if page 1 has been modified since the transaction was started. - ** If it has, check if the schema cookie value (the 4 bytes beginning at - ** byte offset 40) is the same as it is on pPage1. If not, this indicates - ** that the current head of the wal file uses a different schema than - ** the snapshot against which the current transaction was prepared. Return - ** SQLITE_BUSY_SNAPSHOT in this case. */ - if( rc==SQLITE_OK && bSeenPage1==0 ){ - u32 iSlot = 0; - rc = walFindFrame(pWal, 1, iLast, &iSlot); - if( rc==SQLITE_OK && iSlot>pWal->hdr.mxFrame ){ - u8 aNew[4]; - u8 *aOld = &((u8*)pPage1->pData)[40]; - int sz; - i64 iOffset; - sz = pWal->hdr.szPage; - sz = (sz&0xfe00) + ((sz&0x0001)<<16); - iOffset = walFrameOffset(iSlot, sz) + WAL_FRAME_HDRSIZE + 40; - rc = sqlite3OsRead(pWal->pWalFd, aNew, sizeof(aNew), iOffset); - if( rc==SQLITE_OK && memcmp(aOld, aNew, sizeof(aNew)) ){ - /* TODO: New error code? SQLITE_BUSY_SCHEMA. */ - rc = SQLITE_BUSY_SNAPSHOT; + ** has exclusive access to the live header, but might it be corrupt? + ** This code should check that the wal-index-header is Ok, and return + ** SQLITE_BUSY_SNAPSHOT if it is not. */ + int iHash; + int iLastHash = walFramePage(pHead->mxFrame); + for(iHash=walFramePage(pWal->hdr.mxFrame+1); iHash<=iLastHash; iHash++){ + volatile ht_slot *aHash; + volatile u32 *aPgno; + u32 iZero; + + rc = walHashGet(pWal, iHash, &aHash, &aPgno, &iZero); + if( rc==SQLITE_OK ){ + int i; + int iMin = (pWal->hdr.mxFrame+1 - iZero); + int iMax = (iHash==0) ? HASHTABLE_NPAGE_ONE : HASHTABLE_NPAGE; + if( iMin<1 ) iMin = 1; + if( iMax>pHead->mxFrame ) iMax = pHead->mxFrame; + for(i=iMin; i<=iMax; i++){ + if( sqlite3BitvecTestNotNull(pAllRead, aPgno[i]) ){ + sqlite3_log(SQLITE_OK, + "cannot commit UNLOCKED transaction (conflict at page %d)", + (int)aPgno[i] + ); + rc = SQLITE_BUSY_SNAPSHOT; + } } } + if( rc!=SQLITE_OK ) break; } } } @@ -3293,13 +3279,6 @@ int sqlite3WalHeapMemory(Wal *pWal){ return (pWal && pWal->exclusiveMode==WAL_HEAPMEMORY_MODE ); } -/* -** Return true if in a write transaction, false otherwise. -*/ -int sqlite3WalIsInTrans(Wal *pWal){ - return (int)pWal->writeLock; -} - #ifdef SQLITE_ENABLE_ZIPVFS /* ** If the argument is not NULL, it points to a Wal object that holds a diff --git a/src/wal.h b/src/wal.h index db341caa04..fe85a52cc0 100644 --- a/src/wal.h +++ b/src/wal.h @@ -127,8 +127,7 @@ int sqlite3WalExclusiveMode(Wal *pWal, int op); int sqlite3WalHeapMemory(Wal *pWal); /* Return true if the WRITER lock is held. False otherwise. */ -int sqlite3WalIsInTrans(Wal *pWal); -int sqlite3WalLockForCommit(Wal *pWal, PgHdr *pDirtyList, PgHdr *pPage1); +int sqlite3WalLockForCommit(Wal *pWal, Bitvec *pRead); int sqlite3WalCommitRequiresUpgrade(Wal *pWal); #ifdef SQLITE_ENABLE_ZIPVFS diff --git a/test/unlocked.test b/test/unlocked.test index c7ae0466e5..2a5862d3ac 100644 --- a/test/unlocked.test +++ b/test/unlocked.test @@ -300,13 +300,12 @@ do_multiclient_test tn { catch { sql1 ROLLBACK } #----------------------------------------------------------------------- - # The "schema cookie" issue. # # 1. Begin an UNLOCKED write to "t1" using [db] # - # 2. Lots of inserts into t2. Enough to grow the db file. + # 2. Lots of inserts into t2. Enough to grow the db file and modify page 1. # - # 3. Check that the UNLOCKED transaction can still be committed. + # 3. Check that the UNLOCKED transaction can not be committed. # do_test 2.$tn.6.1 { sql1 { @@ -324,12 +323,17 @@ do_multiclient_test tn { } {} do_test 2.$tn.6.3 { + sql1 { SELECT count(*) FROM t2 } + list [catch { sql1 { COMMIT } } msg] $msg [sqlite3_errcode db] + } {1 {database is locked} SQLITE_BUSY_SNAPSHOT} + sql1 ROLLBACK + + do_test 2.$tn.6.4 { sql1 { - SELECT count(*) FROM t2; - COMMIT; + SELECT count(*) FROM t1; SELECT count(*) FROM t2; } - } {1 10001} + } {5 10001} #----------------------------------------------------------------------- #