Move [7d30880114] to the trunk. Add optimizations to reduce the number of opcodes used for BEFORE UPDATE triggers.

FossilOrigin-Name: 1b7c5250ccb63182324bfc3f1ea28f17b6db357a

manifest

@@ -1,8 +1,5 @@
------BEGIN PGP SIGNED MESSAGE-----
+C Move\s[7d30880114]\sto\sthe\strunk.\sAdd\soptimizations\sto\sreduce\sthe\snumber\sof\sopcodes\sused\sfor\sBEFORE\sUPDATE\striggers.
-Hash: SHA1
+D 2009-11-27T12:12:34
-
-C Simplifications\sto\sthe\ssqlite3_trace()\sbound\sparameter\ssubstitution\slogic.
-D 2009-11-26T14:01:53
 F Makefile.arm-wince-mingw32ce-gcc fcd5e9cd67fe88836360bb4f9ef4cb7f8e2fb5a0
 F Makefile.in c5827ead754ab32b9585487177c93bb00b9497b3
 F Makefile.linux-gcc d53183f4aa6a9192d249731c90dbdffbd2c68654
@@ -118,7 +115,7 @@ F src/build.c a48e74d24897100017d39ceba5de255e53ec9488
 F src/callback.c 908f3e0172c3d4058f4ca0acd42c637c52e9669f
 F src/complete.c 417df1ef5ea798532bb6290b0cc4265fef82980a
 F src/date.c a79c0a8f219370b972e320741f995a3bef9df33f
-F src/delete.c ec04635d152debdab70d4b30c5516b59282075ea
+F src/delete.c 8b8afb9cd7783d573eae55a3f4208bc0637a2bb8
 F src/expr.c 50385ed51f1cd7f1ab289629cd0f87d5b2fcca52
 F src/fault.c 160a0c015b6c2629d3899ed2daf63d75754a32bb
 F src/fkey.c e2116672a6bd610dc888e27df292ebc7999c9bb0
@@ -163,13 +160,13 @@ F src/pragma.c 6936d7df5e04b9f996f8f320d15e65b6944b2caa
 F src/prepare.c ad90970bba3aead154266d8bb6faf9fbb5233b94
 F src/printf.c 644bc7d59df3dc56d6d8b9a510914bfc6b51bc69
 F src/random.c cd4a67b3953b88019f8cd4ccd81394a8ddfaba50
-F src/resolve.c c52d9e52e11058f4113f6644adc20d3f85141b1d
+F src/resolve.c d052e5c44bab34f83b3c1741aaa07478d18b5dd5
 F src/rowset.c 69afa95a97c524ba6faf3805e717b5b7ae85a697
 F src/select.c 2f9ed7482e7a25b0b127fc41693bbdbe1caf5647
 F src/shell.c f4948cb6d30665d755a6b5e0ec313d1094aab828
 F src/sqlite.h.in 4464e9772122f0447305d425e04d122b6f1bffec
 F src/sqlite3ext.h 69dfb8116af51b84a029cddb3b35062354270c89
-F src/sqliteInt.h f79536c70bba65124b6fda0b09dfc35f1f513f51
+F src/sqliteInt.h f09be5c67f95f3d28d44e5b608b18cab28758ba4
 F src/sqliteLimit.h 3afab2291762b5d09ae20c18feb8e9fa935a60a6
 F src/status.c e651be6b30d397d86384c6867bc016e4913bcac7
 F src/table.c 2cd62736f845d82200acfa1287e33feb3c15d62e
@@ -207,8 +204,8 @@ F src/test_tclvar.c f4dc67d5f780707210d6bb0eb6016a431c04c7fa
 F src/test_thread.c 00fed80690ae7f1525483a35861511c48bc579f2
 F src/test_wsd.c 41cadfd9d97fe8e3e4e44f61a4a8ccd6f7ca8fe9
 F src/tokenize.c 9f7d39da1a1346fa0cf106ea0bf10bb6b8b61ddf
-F src/trigger.c 3c48db13db94f3c34b01b7caae2130279c8f4d28
+F src/trigger.c d46f9389e3bf3dd1cc1d288aba2f289c96b34200
-F src/update.c 8efeb09822886e33c265dd96d29a3d865ea6dcf2
+F src/update.c edf5649ffc9fca6d6884e34b6da6358e724919fd
 F src/utf.c dad16adcc0c35ef2437dca125a4b07419d361052
 F src/util.c ad4f03079ba0fe83590d1cc9197e8e4844e38592
 F src/vacuum.c 03309a08d549f9389cc3a3589afd4fadbdaf0679
@@ -706,7 +703,7 @@ F test/trigger8.test 30cb0530bd7c4728055420e3f739aa00412eafa4
 F test/trigger9.test 5b0789f1c5c4600961f8e68511b825b87be53e31
 F test/triggerA.test 0718ad2d9bfef27c7af00e636df79bee6b988da7
 F test/triggerB.test 56780c031b454abac2340dbb3b71ac5c56c3d7fe
-F test/triggerC.test 0acc1d22d9c3d6cf0018bf795d544732a25657dc
+F test/triggerC.test 4083c64d80854d271bad211268a08985f3d61cbd
 F test/types.test 9a825ec8eea4e965d7113b74c76a78bb5240f2ac
 F test/types2.test 3555aacf8ed8dc883356e59efc314707e6247a84
 F test/types3.test a0f66bf12f80fad89493535474f7a6d16fa58150
@@ -777,14 +774,7 @@ F tool/speedtest2.tcl ee2149167303ba8e95af97873c575c3e0fab58ff
 F tool/speedtest8.c 2902c46588c40b55661e471d7a86e4dd71a18224
 F tool/speedtest8inst1.c 293327bc76823f473684d589a8160bde1f52c14e
 F tool/vdbe-compress.tcl d70ea6d8a19e3571d7ab8c9b75cba86d1173ff0f
-P f25558f333637b83f98a649acbb8a0d5dbada9ba
+P cb4b928648504ce29d751834e9ee3b5278dfca65
-R 7297b93801cb1b5f0f2d094aa4774976
+R 38ca3dab21e3d85a61a2ba8f76e8fc34
-U drh
+U dan
-Z 9cc025af73edc8bbc9ede953398b7222
+Z 0eaf554ff6bd1729f2cb11e15acc51b6
------BEGIN PGP SIGNATURE-----
-Version: GnuPG v1.4.6 (GNU/Linux)
-
-iD8DBQFLDopUoxKgR168RlERAiBYAJ9naKwYidFTksJBSSLPAu1eSYKX2QCfe441
-vCSQUiBaqpFdB03sRokfcyc=
-=Cbq4
------END PGP SIGNATURE-----

manifest.uuid

@@ -1 +1 @@
-cb4b928648504ce29d751834e9ee3b5278dfca65
+1b7c5250ccb63182324bfc3f1ea28f17b6db357a

src/delete.c

@@ -496,7 +496,9 @@ void sqlite3GenerateRowDelete(
 
     /* TODO: Could use temporary registers here. Also could attempt to
     ** avoid copying the contents of the rowid register.  */
-    mask = sqlite3TriggerOldmask(pParse, pTrigger, 0, pTab, onconf);
+    mask = sqlite3TriggerColmask(
+        pParse, pTrigger, 0, 0, TRIGGER_BEFORE|TRIGGER_AFTER, pTab, onconf
+    );
     mask |= sqlite3FkOldmask(pParse, pTab);
     iOld = pParse->nMem+1;
     pParse->nMem += (1 + pTab->nCol);

src/resolve.c

@@ -264,6 +264,10 @@ static int lookupName(
             testcase( iCol==31 );
             testcase( iCol==32 );
             pParse->oldmask |= (iCol>=32 ? 0xffffffff : (((u32)1)<<iCol));
+          }else{
+            testcase( iCol==31 );
+            testcase( iCol==32 );
+            pParse->newmask |= (iCol>=32 ? 0xffffffff : (((u32)1)<<iCol));
           }
           pExpr->iColumn = (i16)iCol;
           pExpr->pTab = pTab;

src/sqliteInt.h

@@ -2059,15 +2059,16 @@ struct AutoincInfo {
 ** The Parse.pTriggerPrg list never contains two entries with the same
 ** values for both pTrigger and orconf.
 **
-** The TriggerPrg.oldmask variable is set to a mask of old.* columns
+** The TriggerPrg.aColmask[0] variable is set to a mask of old.* columns
 ** accessed (or set to 0 for triggers fired as a result of INSERT 
-** statements).
+** statements). Similarly, the TriggerPrg.aColmask[1] variable is set to
+** a mask of new.* columns used by the program.
 */
 struct TriggerPrg {
   Trigger *pTrigger;      /* Trigger this program was coded from */
   int orconf;             /* Default ON CONFLICT policy */
   SubProgram *pProgram;   /* Program implementing pTrigger/orconf */
-  u32 oldmask;            /* Mask of old.* columns accessed */
+  u32 aColmask[2];        /* Masks of old.*, new.* columns accessed */
   TriggerPrg *pNext;      /* Next entry in Parse.pTriggerPrg list */
 };
 
@@ -2139,6 +2140,7 @@ struct Parse {
   Parse *pToplevel;    /* Parse structure for main program (or NULL) */
   Table *pTriggerTab;  /* Table triggers are being coded for */
   u32 oldmask;         /* Mask of old.* columns referenced */
+  u32 newmask;         /* Mask of new.* columns referenced */
   u8 eTriggerOp;       /* TK_UPDATE, TK_INSERT or TK_DELETE */
   u8 eOrconf;          /* Default ON CONFLICT policy for trigger steps */
   u8 disableTriggers;  /* True to disable triggers */
@@ -2736,7 +2738,7 @@ void sqlite3MaterializeView(Parse*, Table*, Expr*, int);
   TriggerStep *sqlite3TriggerDeleteStep(sqlite3*,Token*, Expr*);
   void sqlite3DeleteTrigger(sqlite3*, Trigger*);
   void sqlite3UnlinkAndDeleteTrigger(sqlite3*,int,const char*);
-  u32 sqlite3TriggerOldmask(Parse*,Trigger*,ExprList*,Table*,int);
+  u32 sqlite3TriggerColmask(Parse*,Trigger*,ExprList*,int,int,Table*,int);
 # define sqlite3ParseToplevel(p) ((p)->pToplevel ? (p)->pToplevel : (p))
 #else
 # define sqlite3TriggersExist(B,C,D,E,F) 0
@@ -2747,7 +2749,7 @@ void sqlite3MaterializeView(Parse*, Table*, Expr*, int);
 # define sqlite3CodeRowTriggerDirect(A,B,C,D,E,F)
 # define sqlite3TriggerList(X, Y) 0
 # define sqlite3ParseToplevel(p) p
-# define sqlite3TriggerOldmask(A,B,C,D,E) 0
+# define sqlite3TriggerColmask(A,B,C,D,E,F,G) 0
 #endif
 
 int sqlite3JoinType(Parse*, Token*, Token*, Token*);

src/trigger.c

@@ -811,7 +811,8 @@ static TriggerPrg *codeRowTrigger(
   pProgram->nRef = 1;
   pPrg->pTrigger = pTrigger;
   pPrg->orconf = orconf;
-  pPrg->oldmask = 0xffffffff;
+  pPrg->aColmask[0] = 0xffffffff;
+  pPrg->aColmask[1] = 0xffffffff;
 
   /* Allocate and populate a new Parse context to use for coding the 
   ** trigger sub-program.  */
@@ -872,7 +873,8 @@ static TriggerPrg *codeRowTrigger(
     pProgram->nMem = pSubParse->nMem;
     pProgram->nCsr = pSubParse->nTab;
     pProgram->token = (void *)pTrigger;
-    pPrg->oldmask = pSubParse->oldmask;
+    pPrg->aColmask[0] = pSubParse->oldmask;
+    pPrg->aColmask[1] = pSubParse->newmask;
     sqlite3VdbeDelete(v);
   }
 
@@ -1032,28 +1034,36 @@ void sqlite3CodeRowTrigger(
 }
 
 /*
-** Triggers fired by UPDATE or DELETE statements may access values stored
+** Triggers may access values stored in the old.* or new.* pseudo-table. 
-** in the old.* pseudo-table. This function returns a 32-bit bitmask
+** This function returns a 32-bit bitmask indicating which columns of the 
-** indicating which columns of the old.* table actually are used by
+** old.* or new.* tables actually are used by triggers. This information 
-** triggers. This information may be used by the caller to avoid having
+** may be used by the caller, for example, to avoid having to load the entire
-** to load the entire old.* record into memory when executing an UPDATE
+** old.* record into memory when executing an UPDATE or DELETE command.
-** or DELETE command.
 **
 ** Bit 0 of the returned mask is set if the left-most column of the
-** table may be accessed using an old.<col> reference. Bit 1 is set if
+** table may be accessed using an [old|new].<col> reference. Bit 1 is set if
 ** the second leftmost column value is required, and so on. If there
 ** are more than 32 columns in the table, and at least one of the columns
 ** with an index greater than 32 may be accessed, 0xffffffff is returned.
 **
-** It is not possible to determine if the old.rowid column is accessed
+** It is not possible to determine if the old.rowid or new.rowid column is 
-** by triggers. The caller must always assume that it is.
+** accessed by triggers. The caller must always assume that it is.
 **
-** There is no equivalent function for new.* references.
+** Parameter isNew must be either 1 or 0. If it is 0, then the mask returned
+** applies to the old.* table. If 1, the new.* table.
+**
+** Parameter tr_tm must be a mask with one or both of the TRIGGER_BEFORE
+** and TRIGGER_AFTER bits set. Values accessed by BEFORE triggers are only
+** included in the returned mask if the TRIGGER_BEFORE bit is set in the
+** tr_tm parameter. Similarly, values accessed by AFTER triggers are only
+** included in the returned mask if the TRIGGER_AFTER bit is set in tr_tm.
 */
-u32 sqlite3TriggerOldmask(
+u32 sqlite3TriggerColmask(
   Parse *pParse,       /* Parse context */
   Trigger *pTrigger,   /* List of triggers on table pTab */
   ExprList *pChanges,  /* Changes list for any UPDATE OF triggers */
+  int isNew,           /* 1 for new.* ref mask, 0 for old.* ref mask */
+  int tr_tm,           /* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */
   Table *pTab,         /* The table to code triggers from */
   int orconf           /* Default ON CONFLICT policy for trigger steps */
 ){
@@ -1061,12 +1071,15 @@ u32 sqlite3TriggerOldmask(
   u32 mask = 0;
   Trigger *p;
 
+  assert( isNew==1 || isNew==0 );
   for(p=pTrigger; p; p=p->pNext){
-    if( p->op==op && checkColumnOverlap(p->pColumns,pChanges) ){
+    if( p->op==op && (tr_tm&p->tr_tm)
+     && checkColumnOverlap(p->pColumns,pChanges)
+    ){
       TriggerPrg *pPrg;
       pPrg = getRowTrigger(pParse, p, pTab, orconf);
       if( pPrg ){
-        mask |= pPrg->oldmask;
+        mask |= pPrg->aColmask[isNew];
       }
     }
   }

src/update.c

@@ -111,14 +111,15 @@ void sqlite3Update(
   AuthContext sContext;  /* The authorization context */
   NameContext sNC;       /* The name-context to resolve expressions in */
   int iDb;               /* Database containing the table being updated */
-  int j1;                /* Addresses of jump instructions */
   int okOnePass;         /* True for one-pass algorithm without the FIFO */
   int hasFK;             /* True if foreign key processing is required */
 
 #ifndef SQLITE_OMIT_TRIGGER
-  int isView;                  /* Trying to update a view */
+  int isView;            /* True when updating a view (INSTEAD OF trigger) */
-  Trigger *pTrigger;           /* List of triggers on pTab, if required */
+  Trigger *pTrigger;     /* List of triggers on pTab, if required */
+  int tmask;             /* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */
 #endif
+  int newmask;
 
   /* Register Allocations */
   int regRowCount = 0;   /* A count of rows changed */
@@ -146,11 +147,13 @@ void sqlite3Update(
   ** updated is a view.
   */
 #ifndef SQLITE_OMIT_TRIGGER
-  pTrigger = sqlite3TriggersExist(pParse, pTab, TK_UPDATE, pChanges, 0);
+  pTrigger = sqlite3TriggersExist(pParse, pTab, TK_UPDATE, pChanges, &tmask);
   isView = pTab->pSelect!=0;
+  assert( pTrigger || tmask==0 );
 #else
 # define pTrigger 0
 # define isView 0
+# define tmask 0
 #endif
 #ifdef SQLITE_OMIT_VIEW
 # undef isView
@@ -160,7 +163,7 @@ void sqlite3Update(
   if( sqlite3ViewGetColumnNames(pParse, pTab) ){
     goto update_cleanup;
   }
-  if( sqlite3IsReadOnly(pParse, pTab, (pTrigger?1:0)) ){
+  if( sqlite3IsReadOnly(pParse, pTab, tmask) ){
     goto update_cleanup;
   }
   aXRef = sqlite3DbMallocRaw(db, sizeof(int) * pTab->nCol );
@@ -388,7 +391,9 @@ void sqlite3Update(
   ** with the required old.* column data.  */
   if( hasFK || pTrigger ){
     u32 oldmask = (hasFK ? sqlite3FkOldmask(pParse, pTab) : 0);
-    oldmask |= sqlite3TriggerOldmask(pParse, pTrigger, pChanges, pTab, onError);
+    oldmask |= sqlite3TriggerColmask(pParse, 
+        pTrigger, pChanges, 0, TRIGGER_BEFORE|TRIGGER_AFTER, pTab, onError
+    );
     for(i=0; i<pTab->nCol; i++){
       if( aXRef[i]<0 || oldmask==0xffffffff || (oldmask & (1<<i)) ){
         sqlite3VdbeAddOp3(v, OP_Column, iCur, i, regOld+i);
@@ -405,24 +410,42 @@ void sqlite3Update(
   /* Populate the array of registers beginning at regNew with the new
   ** row data. This array is used to check constaints, create the new
   ** table and index records, and as the values for any new.* references
-  ** made by triggers.  */
+  ** made by triggers.
+  **
+  ** If there are one or more BEFORE triggers, then do not populate the
+  ** registers associated with columns that are (a) not modified by
+  ** this UPDATE statement and (b) not accessed by new.* references. The
+  ** values for registers not modified by the UPDATE must be reloaded from 
+  ** the database after the BEFORE triggers are fired anyway (as the trigger 
+  ** may have modified them). So not loading those that are not going to
+  ** be used eliminates some redundant opcodes.
+  */
+  newmask = sqlite3TriggerColmask(
+      pParse, pTrigger, pChanges, 1, TRIGGER_BEFORE, pTab, onError
+  );
   for(i=0; i<pTab->nCol; i++){
     if( i==pTab->iPKey ){
       sqlite3VdbeAddOp2(v, OP_Null, 0, regNew+i);
     }else{
       j = aXRef[i];
-      if( j<0 ){
+      if( j>=0 ){
+        sqlite3ExprCode(pParse, pChanges->a[j].pExpr, regNew+i);
+      }else if( 0==(tmask&TRIGGER_BEFORE) || i>31 || (newmask&(1<<i)) ){
+        /* This branch loads the value of a column that will not be changed 
+        ** into a register. This is done if there are no BEFORE triggers, or
+        ** if there are one or more BEFORE triggers that use this value via
+        ** a new.* reference in a trigger program.
+        */
         sqlite3VdbeAddOp3(v, OP_Column, iCur, i, regNew+i);
         sqlite3ColumnDefault(v, pTab, i, regNew+i);
-      }else{
-        sqlite3ExprCode(pParse, pChanges->a[j].pExpr, regNew+i);
       }
     }
   }
 
   /* Fire any BEFORE UPDATE triggers. This happens before constraints are
-  ** verified. One could argue that this is wrong.  */
+  ** verified. One could argue that this is wrong.
-  if( pTrigger ){
+  */
+  if( tmask&TRIGGER_BEFORE ){
     sqlite3VdbeAddOp2(v, OP_Affinity, regNew, pTab->nCol);
     sqlite3TableAffinityStr(v, pTab);
     sqlite3CodeRowTrigger(pParse, pTrigger, TK_UPDATE, pChanges, 
@@ -432,11 +455,25 @@ void sqlite3Update(
     ** case, jump to the next row. No updates or AFTER triggers are 
     ** required. This behaviour - what happens when the row being updated
     ** is deleted or renamed by a BEFORE trigger - is left undefined in the
-    ** documentation.  */
+    ** documentation.
+    */
     sqlite3VdbeAddOp3(v, OP_NotExists, iCur, addr, regOldRowid);
+
+    /* If it did not delete it, the row-trigger may still have modified 
+    ** some of the columns of the row being updated. Load the values for 
+    ** all columns not modified by the update statement into their 
+    ** registers in case this has happened.
+    */
+    for(i=0; i<pTab->nCol; i++){
+      if( aXRef[i]<0 && i!=pTab->iPKey ){
+        sqlite3VdbeAddOp3(v, OP_Column, iCur, i, regNew+i);
+        sqlite3ColumnDefault(v, pTab, i, regNew+i);
+      }
+    }
   }
 
   if( !isView ){
+    int j1;                       /* Address of jump instruction */
 
     /* Do constraint checks. */
     sqlite3GenerateConstraintChecks(pParse, pTab, iCur, regNewRowid,

test/triggerC.test

@@ -784,8 +784,76 @@ do_test triggerC-9.2 {
     SELECT a FROM t9 ORDER BY a;
   }
 } {1 2 3 4}
- 
 
+# At one point (between versions 3.6.18 and 3.6.20 inclusive), an UPDATE 
+# that fired a BEFORE trigger that itself updated the same row as the 
+# statement causing it to fire was causing a strange side-effect: The 
+# values updated by the statement within the trigger were being overwritten 
+# by the values in the new.* array, even if those values were not 
+# themselves written by the parent UPDATE statement.
+#
+# Technically speaking this was not a bug. The SQLite documentation says
+# that if a BEFORE UPDATE or BEFORE DELETE trigger modifies or deletes the
+# row that the parent statement is operating on the results are undefined. 
+# But as of 3.6.21 behaviour is restored to the way it was in versions
+# 3.6.17 and earlier to avoid causing unnecessary difficulties.
+#
+do_test triggerC-10.1 {
+  execsql {
+    CREATE TABLE t10(a, updatecnt DEFAULT 0);
+    CREATE TRIGGER t10_bu BEFORE UPDATE OF a ON t10 BEGIN
+      UPDATE t10 SET updatecnt = updatecnt+1 WHERE rowid = old.rowid;
+    END;
+    INSERT INTO t10(a) VALUES('hello');
+  }
+
+  # Before the problem was fixed, table t10 would contain the tuple 
+  # (world, 0) after running the following script (because the value
+  # 1 written to column "updatecnt" was clobbered by the old value 0).
+  #
+  execsql {
+    UPDATE t10 SET a = 'world';
+    SELECT * FROM t10;
+  }
+} {world 1}
+
+do_test triggerC-10.2 {
+  execsql {
+    UPDATE t10 SET a = 'tcl', updatecnt = 5;
+    SELECT * FROM t10;
+  }
+} {tcl 5}
+
+do_test triggerC-10.3 {
+  execsql {
+    CREATE TABLE t11(
+      c1,   c2,  c3,  c4,  c5,  c6,  c7,  c8,  c9, c10,
+      c11, c12, c13, c14, c15, c16, c17, c18, c19, c20,
+      c21, c22, c23, c24, c25, c26, c27, c28, c29, c30,
+      c31, c32, c33, c34, c35, c36, c37, c38, c39, c40
+    );
+
+    CREATE TRIGGER t11_bu BEFORE UPDATE OF c1 ON t11 BEGIN
+      UPDATE t11 SET c31 = c31+1, c32=c32+1 WHERE rowid = old.rowid;
+    END;
+
+    INSERT INTO t11 VALUES(
+      1,   2,  3,  4,  5,  6,  7,  8,  9, 10,
+      11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
+      21, 22, 23, 24, 25, 26, 27, 28, 29, 30,
+      31, 32, 33, 34, 35, 36, 37, 38, 39, 40
+    );
+  }
+
+  # Before the problem was fixed, table t10 would contain the tuple 
+  # (world, 0) after running the following script (because the value
+  # 1 written to column "updatecnt" was clobbered by the old value 0).
+  #
+  execsql {
+    UPDATE t11 SET c4=35, c33=22, c1=5;
+    SELECT * FROM t11;
+  } 
+} {5 2 3 35 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 32 33 22 34 35 36 37 38 39 40}
 
 
 finish_test