From: drh <drh@noemail.net>
Date: Tue, 19 May 2020 12:27:29 +0000 (+0000)
Subject: In wal.c, improved comments on concurrency issues.  More use of
X-Git-Tag: version-3.32.0~7
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f16cf653f44115821c4718a255f8be6d8d594baa;p=thirdparty%2Fsqlite.git

In wal.c, improved comments on concurrency issues.  More use of
AtomicLoad() and AtomicStore().

FossilOrigin-Name: 4bf566feca3a8fbe5e386533aac30e0ac25836cfc820a3abd91e156bd6198b4a
---

diff --git a/manifest b/manifest
index 9f4f6e3da8..26d7750597 100644
--- a/manifest
+++ b/manifest
@@ -1,5 +1,5 @@
-C Add\svdbevtab.lo\sto\sbuild\sof\slibsqlite3.lib\sin\sMakefile.msc.
-D 2020-05-18T18:02:37.420
+C In\swal.c,\simproved\scomments\son\sconcurrency\sissues.\s\sMore\suse\sof\nAtomicLoad()\sand\sAtomicStore().
+D 2020-05-19T12:27:29.008
 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1
 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea
 F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724
@@ -617,7 +617,7 @@ F src/vdbetrace.c fa3bf238002f0bbbdfb66cc8afb0cea284ff9f148d6439bc1f6f2b4c3b7143
 F src/vdbevtab.c ee5b4c902fdda2230f9503ac7b84c6d614c91e8f6f4dc1633e2e8dfef8ffb144
 F src/vtab.c 7b452592ed2ee95dedb1f323d557cebede5a6f3b4558b21a5dca527e6ae9b12c
 F src/vxworks.h d2988f4e5a61a4dfe82c6524dd3d6e4f2ce3cdb9
-F src/wal.c d0365581aed11a75e176f67cdbdc1bdfe685c5381a052360a00bbf58d3252fa0
+F src/wal.c fc763af2c20d1770e872378b28031c100b52061ec47599f38499cf4bd4977284
 F src/wal.h c3aa7825bfa2fe0d85bef2db94655f99870a285778baa36307c0a16da32b226a
 F src/walker.c 7c429c694abd12413a5c17aec9f47cfe9eba6807e6b0a32df883e8e3a14835ed
 F src/where.c 9546c82056e8cdb27291f98cf1adca5d271240b399bb97b32f77fc2bea6146c9
@@ -1866,7 +1866,7 @@ F vsixtest/vsixtest.tcl 6a9a6ab600c25a91a7acc6293828957a386a8a93
 F vsixtest/vsixtest.vcxproj.data 2ed517e100c66dc455b492e1a33350c1b20fbcdc
 F vsixtest/vsixtest.vcxproj.filters 37e51ffedcdb064aad6ff33b6148725226cd608e
 F vsixtest/vsixtest_TemporaryKey.pfx e5b1b036facdb453873e7084e1cae9102ccc67a0
-P 69e149f76853d196c8855fedfc98848b60fb116ac36bc08824b1a122469f8ece
-R 3630bc41d5bae3aeb507521e3969369e
-U dan
-Z ad12d06ec98773d9201d7cf643b16e9f
+P f64d054b799c0af8d582fd9aff9bcbfa380ef08026ba5db8c5f89f6f56dfdad9
+R 7f686b5b79d2dddadbc47e9d743b06cb
+U drh
+Z 8798d48c38c827000797710a250a5723
diff --git a/manifest.uuid b/manifest.uuid
index 7c14e40f17..09b77250a7 100644
--- a/manifest.uuid
+++ b/manifest.uuid
@@ -1 +1 @@
-f64d054b799c0af8d582fd9aff9bcbfa380ef08026ba5db8c5f89f6f56dfdad9
\ No newline at end of file
+4bf566feca3a8fbe5e386533aac30e0ac25836cfc820a3abd91e156bd6198b4a
\ No newline at end of file
diff --git a/src/wal.c b/src/wal.c
index e8f7f24f85..3816883686 100644
--- a/src/wal.c
+++ b/src/wal.c
@@ -689,6 +689,10 @@ static void walChecksumBytes(
   aOut[1] = s2;
 }
 
+/*
+** If there is the possibility of concurrent access to the SHM file
+** from multiple threads and/or processes, then do a memory barrier.
+*/
 static void walShmBarrier(Wal *pWal){
   if( pWal->exclusiveMode!=WAL_HEAPMEMORY_MODE ){
     sqlite3OsShmBarrier(pWal->pDbFd);
@@ -708,6 +712,7 @@ static void walIndexWriteHdr(Wal *pWal){
   pWal->hdr.isInit = 1;
   pWal->hdr.iVersion = WALINDEX_MAX_VERSION;
   walChecksumBytes(1, (u8*)&pWal->hdr, nCksum, 0, pWal->hdr.aCksum);
+  /* Possible TSAN false-positive.  See tag-20200519-1 */
   memcpy((void*)&aHdr[1], (const void*)&pWal->hdr, sizeof(WalIndexHdr));
   walShmBarrier(pWal);
   memcpy((void*)&aHdr[0], (const void*)&pWal->hdr, sizeof(WalIndexHdr));
@@ -1897,32 +1902,13 @@ static int walCheckpoint(
     mxSafeFrame = pWal->hdr.mxFrame;
     mxPage = pWal->hdr.nPage;
     for(i=1; i<WAL_NREADER; i++){
-      /* Thread-sanitizer reports that the following is an unsafe read,
-      ** as some other thread may be in the process of updating the value
-      ** of the aReadMark[] slot. The assumption here is that if that is
-      ** happening, the other client may only be increasing the value,
-      ** not decreasing it. So assuming either that either the "old" or
-      ** "new" version of the value is read, and not some arbitrary value
-      ** that would never be written by a real client, things are still 
-      ** safe.
-      **
-      ** Astute readers have pointed out that the assumption stated in the
-      ** last sentence of the previous paragraph is not guaranteed to be
-      ** true for all conforming systems.  However, the assumption is true
-      ** for all compilers and architectures in common use today (circa
-      ** 2019-11-27) and the alternatives are both slow and complex, and
-      ** so we will continue to go with the current design for now.  If this
-      ** bothers you, or if you really are running on a system where aligned
-      ** 32-bit reads and writes are not atomic, then you can simply avoid
-      ** the use of WAL mode, or only use WAL mode together with
-      ** PRAGMA locking_mode=EXCLUSIVE and all will be well.
-      */
-      u32 y = pInfo->aReadMark[i];
+      u32 y = AtomicLoad(pInfo->aReadMark+i);
       if( mxSafeFrame>y ){
         assert( y<=pWal->hdr.mxFrame );
         rc = walBusyLock(pWal, xBusy, pBusyArg, WAL_READ_LOCK(i), 1);
         if( rc==SQLITE_OK ){
-          pInfo->aReadMark[i] = (i==1 ? mxSafeFrame : READMARK_NOT_USED);
+          u32 iMark = (i==1 ? mxSafeFrame : READMARK_NOT_USED);
+          AtomicStore(pInfo->aReadMark+i, iMark);
           walUnlockExclusive(pWal, WAL_READ_LOCK(i), 1);
         }else if( rc==SQLITE_BUSY ){
           mxSafeFrame = y;
@@ -1940,7 +1926,7 @@ static int walCheckpoint(
     }
 
     if( pIter
-     && (rc = walBusyLock(pWal, xBusy, pBusyArg, WAL_READ_LOCK(0),1))==SQLITE_OK
+     && (rc = walBusyLock(pWal,xBusy,pBusyArg,WAL_READ_LOCK(0),1))==SQLITE_OK
     ){
       u32 nBackfill = pInfo->nBackfill;
 
@@ -2168,13 +2154,19 @@ static int walIndexTryHdr(Wal *pWal, int *pChanged){
   ** meaning it is possible that an inconsistent snapshot is read
   ** from the file. If this happens, return non-zero.
   **
+  ** tag-20200519-1:
   ** There are two copies of the header at the beginning of the wal-index.
   ** When reading, read [0] first then [1].  Writes are in the reverse order.
   ** Memory barriers are used to prevent the compiler or the hardware from
-  ** reordering the reads and writes.
+  ** reordering the reads and writes.  TSAN and similar tools can sometimes
+  ** give false-positive warnings about these accesses because the tools do not
+  ** account for the double-read and the memory barrier. The use of mutexes
+  ** here would be problematic as the memory being accessed is potentially
+  ** shared among multiple processes and not all mutex implementions work
+  ** reliably in that environment.
   */
   aHdr = walIndexHdr(pWal);
-  memcpy(&h1, (void *)&aHdr[0], sizeof(h1));
+  memcpy(&h1, (void *)&aHdr[0], sizeof(h1)); /* Possible TSAN false-positive */
   walShmBarrier(pWal);
   memcpy(&h2, (void *)&aHdr[1], sizeof(h2));
 
@@ -3015,14 +3007,15 @@ int sqlite3WalFindFrame(
     int iKey;                     /* Hash slot index */
     int nCollide;                 /* Number of hash collisions remaining */
     int rc;                       /* Error code */
+    u32 iH;
 
     rc = walHashGet(pWal, iHash, &sLoc);
     if( rc!=SQLITE_OK ){
       return rc;
     }
     nCollide = HASHTABLE_NSLOT;
-    for(iKey=walHash(pgno); sLoc.aHash[iKey]; iKey=walNextHash(iKey)){
-      u32 iH = sLoc.aHash[iKey];
+    iKey = walHash(pgno);
+    while( (iH = AtomicLoad(&sLoc.aHash[iKey]))!=0 ){
       u32 iFrame = iH + sLoc.iZero;
       if( iFrame<=iLast && iFrame>=pWal->minFrame && sLoc.aPgno[iH]==pgno ){
         assert( iFrame>iRead || CORRUPT_DB );
@@ -3031,6 +3024,7 @@ int sqlite3WalFindFrame(
       if( (nCollide--)==0 ){
         return SQLITE_CORRUPT_BKPT;
       }
+      iKey = walNextHash(iKey);
     }
     if( iRead ) break;
   }