Reorganize some multi-threaded code in vdbesort.c so that full MC/DC test coverage does not depend on the outcome of a race condition.

FossilOrigin-Name: 78c7ec95931265b89a92f6a799fc9b1a9f0476bf

manifest

@@ -1,5 +1,5 @@
-C Cleanup\sof\sthe\ssqlite3StrAccumInit()\sfunction.\s\sNo\sfunctionality\schanges.
+C Reorganize\ssome\smulti-threaded\scode\sin\svdbesort.c\sso\sthat\sfull\sMC/DC\stest\scoverage\sdoes\snot\sdepend\son\sthe\soutcome\sof\sa\srace\scondition.
-D 2015-05-02T11:45:53.551
+D 2015-05-02T12:40:12.793
 F Makefile.arm-wince-mingw32ce-gcc d6df77f1f48d690bd73162294bbba7f59507c72f
 F Makefile.in e628c50e237251fc7e768bef14ee7e822ad69e69
 F Makefile.linux-gcc 91d710bdc4998cb015f39edf3cb314ec4f4d7e23
@@ -300,7 +300,7 @@ F src/vdbeapi.c 583d56b129dd27f12bed518270de9ebe521e6a75
 F src/vdbeaux.c 03591cca98ec50e1493043f0ff7abbece0b9c83d
 F src/vdbeblob.c 4f2e8e075d238392df98c5e03a64342465b03f90
 F src/vdbemem.c 7bfbeef0978a2e1a05d979641fdbf7c189b7ddf4
-F src/vdbesort.c 2e7f683464fd5db3be4beaa1ff2d39e24fcb64b8
+F src/vdbesort.c f5009e7a35e3065635d8918b9a31f498a499976b
 F src/vdbetrace.c 8befe829faff6d9e6f6e4dee5a7d3f85cc85f1a0
 F src/vtab.c c535e80259ebe616467181a83a4263555b97c694
 F src/vxworks.h c18586c8edc1bddbc15c004fa16aeb1e1342b4fb
@@ -1256,7 +1256,7 @@ F tool/vdbe_profile.tcl 67746953071a9f8f2f668b73fe899074e2c6d8c1
 F tool/warnings-clang.sh f6aa929dc20ef1f856af04a730772f59283631d4
 F tool/warnings.sh 0abfd78ceb09b7f7c27c688c8e3fe93268a13b32
 F tool/win/sqlite.vsix deb315d026cc8400325c5863eef847784a219a2f
-P 04630b989d8794b9ed2553f4d223de2b322437c5
+P 7952c32268aa650d9ee946d5bfe190f712e3bbe6
-R cd7508b548af68bb962d66cc8239f85e
+R b89595a534f421de5f0d98acb44972ee
-U drh
+U dan
-Z 85d38ece2e515fea582923b194ee7234
+Z 22691f754ec0b999bb5069d3c75c8ca1

manifest.uuid

@@ -1 +1 @@
-7952c32268aa650d9ee946d5bfe190f712e3bbe6
+78c7ec95931265b89a92f6a799fc9b1a9f0476bf

src/vdbesort.c

@@ -2063,11 +2063,12 @@ static void vdbeMergeEngineCompare(
 #define INCRINIT_TASK   1
 #define INCRINIT_ROOT   2
 
-/* Forward reference.
+/* 
-** The vdbeIncrMergeInit() and vdbePmaReaderIncrMergeInit() routines call each
+** Forward reference required as the vdbeIncrMergeInit() and
-** other (when building a merge tree).
+** vdbePmaReaderIncrInit() routines are called mutually recursively when
+** building a merge tree.
 */
-static int vdbePmaReaderIncrMergeInit(PmaReader *pReadr, int eMode);
+static int vdbePmaReaderIncrInit(PmaReader *pReadr, int eMode);
 
 /*
 ** Initialize the MergeEngine object passed as the second argument. Once this
@@ -2114,7 +2115,7 @@ static int vdbeMergeEngineInit(
       ** better advantage of multi-processor hardware. */
       rc = vdbePmaReaderNext(&pMerger->aReadr[nTree-i-1]);
     }else{
-      rc = vdbePmaReaderIncrMergeInit(&pMerger->aReadr[i], INCRINIT_NORMAL);
+      rc = vdbePmaReaderIncrInit(&pMerger->aReadr[i], INCRINIT_NORMAL);
     }
     if( rc!=SQLITE_OK ) return rc;
   }
@@ -2126,17 +2127,15 @@ static int vdbeMergeEngineInit(
 }
 
 /*
-** Initialize the IncrMerge field of a PmaReader.
+** The PmaReader passed as the first argument is guaranteed to be an
-**
+** incremental-reader (pReadr->pIncr!=0). This function serves to open
-** If the PmaReader passed as the first argument is not an incremental-reader
+** and/or initialize the temp file related fields of the IncrMerge
-** (if pReadr->pIncr==0), then this function is a no-op. Otherwise, it serves
-** to open and/or initialize the temp file related fields of the IncrMerge
 ** object at (pReadr->pIncr).
 **
 ** If argument eMode is set to INCRINIT_NORMAL, then all PmaReaders
-** in the sub-tree headed by pReadr are also initialized. Data is then loaded
+** in the sub-tree headed by pReadr are also initialized. Data is then 
-** into the buffers belonging to pReadr and it is set to
+** loaded into the buffers belonging to pReadr and it is set to point to 
-** point to the first key in its range.
+** the first key in its range.
 **
 ** If argument eMode is set to INCRINIT_TASK, then pReadr is guaranteed
 ** to be a multi-threaded PmaReader and this function is being called in a
@@ -2163,14 +2162,12 @@ static int vdbeMergeEngineInit(
 static int vdbePmaReaderIncrMergeInit(PmaReader *pReadr, int eMode){
   int rc = SQLITE_OK;
   IncrMerger *pIncr = pReadr->pIncr;
+  SortSubtask *pTask = pIncr->pTask;
+  sqlite3 *db = pTask->pSorter->db;
 
   /* eMode is always INCRINIT_NORMAL in single-threaded mode */
   assert( SQLITE_MAX_WORKER_THREADS>0 || eMode==INCRINIT_NORMAL );
 
-  if( pIncr ){
-    SortSubtask *pTask = pIncr->pTask;
-    sqlite3 *db = pTask->pSorter->db;
-
   rc = vdbeMergeEngineInit(pTask, pIncr->pMerger, eMode);
 
   /* Set up the required files for pIncr. A multi-theaded IncrMerge object
@@ -2203,19 +2200,24 @@ static int vdbePmaReaderIncrMergeInit(PmaReader *pReadr, int eMode){
 #if SQLITE_MAX_WORKER_THREADS>0
   if( rc==SQLITE_OK && pIncr->bUseThread ){
     /* Use the current thread to populate aFile[1], even though this
-      ** PmaReader is multi-threaded. The reason being that this function
+    ** PmaReader is multi-threaded. If this is an INCRINIT_TASK object,
-      ** is already running in background thread pIncr->pTask->thread. */
+    ** then this function is already running in background thread 
+    ** pIncr->pTask->thread. 
+    **
+    ** If this is the INCRINIT_ROOT object, then it is running in the 
+    ** main VDBE thread. But that is Ok, as that thread cannot return
+    ** control to the VDBE or proceed with anything useful until the 
+    ** first results are ready from this merger object anyway.
+    */
     assert( eMode==INCRINIT_ROOT || eMode==INCRINIT_TASK );
     rc = vdbeIncrPopulate(pIncr);
   }
 #endif
 
-    if( rc==SQLITE_OK
+  if( rc==SQLITE_OK && (SQLITE_MAX_WORKER_THREADS==0 || eMode!=INCRINIT_TASK) ){
-     && (SQLITE_MAX_WORKER_THREADS==0 || eMode!=INCRINIT_TASK)
-    ){
     rc = vdbePmaReaderNext(pReadr);
   }
-  }
+
   return rc;
 }
 
@@ -2224,7 +2226,7 @@ static int vdbePmaReaderIncrMergeInit(PmaReader *pReadr, int eMode){
 ** The main routine for vdbePmaReaderIncrMergeInit() operations run in 
 ** background threads.
 */
-static void *vdbePmaReaderBgInit(void *pCtx){
+static void *vdbePmaReaderBgIncrInit(void *pCtx){
   PmaReader *pReader = (PmaReader*)pCtx;
   void *pRet = SQLITE_INT_TO_PTR(
                   vdbePmaReaderIncrMergeInit(pReader,INCRINIT_TASK)
@@ -2232,20 +2234,36 @@ static void *vdbePmaReaderBgInit(void *pCtx){
   pReader->pIncr->pTask->bDone = 1;
   return pRet;
 }
+#endif
 
 /*
-** Use a background thread to invoke vdbePmaReaderIncrMergeInit(INCRINIT_TASK) 
+** If the PmaReader passed as the first argument is not an incremental-reader
-** on the PmaReader object passed as the first argument.
+** (if pReadr->pIncr==0), then this function is a no-op. Otherwise, it invokes
+** the vdbePmaReaderIncrMergeInit() function with the parameters passed to
+** this routine to initialize the incremental merge.
 ** 
-** This call will initialize the various fields of the pReadr->pIncr 
+** If the IncrMerger object is multi-threaded (IncrMerger.bUseThread==1), 
-** structure and, if it is a multi-threaded IncrMerger, launch a 
+** then a background thread is launched to call vdbePmaReaderIncrMergeInit().
-** background thread to populate aFile[1].
+** Or, if the IncrMerger is single threaded, the same function is called
+** using the current thread.
 */
-static int vdbePmaReaderBgIncrInit(PmaReader *pReadr){
+static int vdbePmaReaderIncrInit(PmaReader *pReadr, int eMode){
+  IncrMerger *pIncr = pReadr->pIncr;   /* Incremental merger */
+  int rc = SQLITE_OK;                  /* Return code */
+  if( pIncr ){
+#if SQLITE_MAX_WORKER_THREADS>0
+    assert( pIncr->bUseThread==0 || eMode==INCRINIT_TASK );
+    if( pIncr->bUseThread ){
       void *pCtx = (void*)pReadr;
-  return vdbeSorterCreateThread(pReadr->pIncr->pTask, vdbePmaReaderBgInit, pCtx);
+      rc = vdbeSorterCreateThread(pIncr->pTask, vdbePmaReaderBgIncrInit, pCtx);
-}
+    }else
 #endif
+    {
+      rc = vdbePmaReaderIncrMergeInit(pReadr, eMode);
+    }
+  }
+  return rc;
+}
 
 /*
 ** Allocate a new MergeEngine object to merge the contents of nPMA level-0
@@ -2490,15 +2508,21 @@ static int vdbeSorterSetupMerge(VdbeSorter *pSorter){
             }
           }
           for(iTask=0; rc==SQLITE_OK && iTask<pSorter->nTask; iTask++){
+            /* Check that:
+            **   
+            **   a) The incremental merge object is configured to use the
+            **      right task, and
+            **   b) If it is using task (nTask-1), it is configured to run
+            **      in single-threaded mode. This is important, as the
+            **      root merge (INCRINIT_ROOT) will be using the same task
+            **      object.
+            */
             PmaReader *p = &pMain->aReadr[iTask];
-            assert( p->pIncr==0 || p->pIncr->pTask==&pSorter->aTask[iTask] );
+            assert( p->pIncr==0 || (
-            if( p->pIncr ){ 
+                (p->pIncr->pTask==&pSorter->aTask[iTask])             /* a */
-              if( iTask==pSorter->nTask-1 ){
+             && (iTask!=pSorter->nTask-1 || p->pIncr->bUseThread==0)  /* b */
-                rc = vdbePmaReaderIncrMergeInit(p, INCRINIT_TASK);
+            ));
-              }else{
+            rc = vdbePmaReaderIncrInit(p, INCRINIT_TASK);
-                rc = vdbePmaReaderBgIncrInit(p);
-              }
-            }
           }
         }
         pMain = 0;