Fix UPSERT so that it checks the target-constraint first and fires the

DO UPDATE if that constraint is violated regardless of whether or not
other constraints are in violation.  This aligns SQLite behavior with
what PostgreSQL does. Fix for ticket [908f001483982c43cdb476dfb590a1a].

FossilOrigin-Name: 529fb55e3d00472f13446117527b0896827b11e870b581af7fe7cbb7392ef3cd

manifest

@@ -1,5 +1,5 @@
-C Stop\srequiring\sthe\sglobal\sVFS\smutex\sto\saccess\sthe\sunixInodeInfo.pUnused\sfield.\nThe\sunixInodeInfo\smutex\sis\ssufficient.
+C Fix\sUPSERT\sso\sthat\sit\schecks\sthe\starget-constraint\sfirst\sand\sfires\sthe\nDO\sUPDATE\sif\sthat\sconstraint\sis\sviolated\sregardless\sof\swhether\sor\snot\nother\sconstraints\sare\sin\sviolation.\s\sThis\saligns\sSQLite\sbehavior\swith\nwhat\sPostgreSQL\sdoes.\sFix\sfor\sticket\s[908f001483982c43cdb476dfb590a1a].
-D 2018-08-13T22:50:34.627
+D 2018-08-14T15:12:52.710
 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1
 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea
 F Makefile.in 0a3a6c81e6fcb969ff9106e882f0a08547014ba463cb6beca4c4efaecc924ee6
@@ -459,7 +459,7 @@ F src/hash.c a12580e143f10301ed5166ea4964ae2853d3905a511d4e0c44497245c7ce1f7a
 F src/hash.h ab34c5c54a9e9de2e790b24349ba5aab3dbb4fd4
 F src/hwtime.h 747c1bbe9df21a92e9c50f3bbec1de841dc5e5da
 F src/in-operator.md 10cd8f4bcd225a32518407c2fb2484089112fd71
-F src/insert.c 894594952bcda1dc6e1549871e4022517563545ffc7a3f4e9e5f3faa788893fd
+F src/insert.c c723716f0de7aa0a679300f7d3541c89645f4a9882161cecdb3093fc07f8cc4b
 F src/legacy.c 134ab3e3fae00a0f67a5187981d6935b24b337bcf0f4b3e5c9fa5763da95bf4e
 F src/loadext.c 6aae5739198d96c51ae6eb97c4a5b1744c22ed7a5a565a5399a717780d48a36b
 F src/main.c df233667bbb6f05a8492ea93e0995abbeb816eab53e51e638a0dece1de0e83a3
@@ -570,10 +570,10 @@ F src/utf.c 810fbfebe12359f10bc2a011520a6e10879ab2a163bcb26c74768eab82ea62a5
 F src/util.c d9eb0a6c4aae1b00a7369eadd7ca0bbe946cb4c953b6751aa20d357c2f482157
 F src/vacuum.c 36e7d21a20c0bf6ef4ef7c399d192b5239410b7c4d3c1070fba4e30810d0b855
 F src/vdbe.c b11baa48b293dc48fbd51c6a9029f88bdf4cd117c01225b2a2b5e90e5928a8a3
-F src/vdbe.h d93abdc8bc9295e0a256e582c19f548c545dc498319d108bbc9dd29de31c48a2
+F src/vdbe.h 5081dcc497777efe5e9ebe7330d283a044a005e4bdda2e2e984f03bf89a0d907
 F src/vdbeInt.h 8ea493d994c6697cf7bccc60583a80a0222560490410f60f1113e90d36643ce0
 F src/vdbeapi.c 2ba821c5929a2769e4b217dd85843479c718b8989d414723ec8af0616a83d611
-F src/vdbeaux.c b610cef3d8d381c9287d02c2e61590acc0a1b4de1cc0188d560f5ef345527a24
+F src/vdbeaux.c f03d4a1961ec282abaec5dbf7e5576ddb1eb01e6157335a232d8d9e57fd5eca1
 F src/vdbeblob.c f5c70f973ea3a9e915d1693278a5f890dc78594300cf4d54e64f2b0917c94191
 F src/vdbemem.c 720df42ad8e5c7cb883573de40a185afef4a214903098a16f2bb14b62b2399b7
 F src/vdbesort.c 731a09e5cb9e96b70c394c1b7cf3860fbe84acca7682e178615eb941a3a0ef2f
@@ -1526,7 +1526,7 @@ F test/unixexcl.test d936ba2b06794018e136418addd59a2354eeae97
 F test/unordered.test ffeea7747d5ba962a8009a20b7e53d68cbae05b063604c68702c5998eb50c981
 F test/update.test 1148de8d913e9817717990603aadeca07aab9ddbb10a30f167cbfd8d3a3ccb60
 F test/update2.test 5e67667e1c54017d964e626db765cf8bedcf87483c184f4c575bdb8c1dd2313e
-F test/upsert1.test ecd8f69e20183f298464992762fee24750508b3cbefc5badba8a259a3fc887ec
+F test/upsert1.test 994bde41800bb77dbe32fcd2e1f6c4b49cc9f2c6cd345731c774dff02b51c110
 F test/upsert2.test 9c3cdbb1a890227f6504ce4b0e3de68f4cdfa16bb21d8641208a9239896c5a09
 F test/upsert3.test 88d7d590a1948a9cb6eac1b54b0642f67a9f35a1fc0f19b200e97d5d39e3179c
 F test/upsert4.test 25d2a1da92f149331ae0c51ca6e3eee78189577585eab92de149900d62994fa5
@@ -1754,7 +1754,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 8b1e0010b9e0b548a90087f4d25843d2b40f7e9551722ac587fa925d37b510c2
+P e3ea43dabf099dc2954c23d348638e7b2a8b9122d2994154bc649a2c09260c46
-R c0b44ea86365eec38ab4ca860c436c2d
+R 57b28032d1f7240227e60efc94d5cf5d
 U drh
-Z 61c21fbc1d363c0a3fe7befb1f84f230
+Z f4b7118e2c3ed77a16d581f95e6f4881

manifest.uuid

@@ -1 +1 @@
-e3ea43dabf099dc2954c23d348638e7b2a8b9122d2994154bc649a2c09260c46
+529fb55e3d00472f13446117527b0896827b11e870b581af7fe7cbb7392ef3cd

src/insert.c

@@ -1178,44 +1178,6 @@ static int checkConstraintUnchanged(Expr *pExpr, int *aiChng, int chngRowid){
   return !w.eCode;
 }
 
-/*
-** An instance of the ConstraintAddr object remembers the byte-code addresses
-** for sections of the constraint checks that deal with uniqueness constraints
-** on the rowid and on the upsert constraint.
-**
-** This information is passed into checkReorderConstraintChecks() to insert
-** some OP_Goto operations so that the rowid and upsert constraints occur
-** in the correct order relative to other constraints.
-*/
-typedef struct ConstraintAddr ConstraintAddr;
-struct ConstraintAddr {
-  int ipkTop;          /* Subroutine for rowid constraint check */
-  int upsertTop;       /* Label for upsert constraint check subroutine */
-  int upsertTop2;      /* Copy of upsertTop not cleared by the call */
-  int upsertBtm;       /* upsert constraint returns to this label */
-  int ipkBtm;          /* Return opcode rowid constraint check */
-};
-
-/*
-** Generate any OP_Goto operations needed to cause constraints to be
-** run that haven't already been run.
-*/
-static void reorderConstraintChecks(Vdbe *v, ConstraintAddr *p){
-  if( p->upsertTop ){
-    testcase( sqlite3VdbeLabelHasBeenResolved(v, p->upsertTop) );
-    sqlite3VdbeGoto(v, p->upsertTop);
-    VdbeComment((v, "call upsert subroutine"));
-    sqlite3VdbeResolveLabel(v, p->upsertBtm);
-    p->upsertTop = 0;
-  }
-  if( p->ipkTop ){
-    sqlite3VdbeGoto(v, p->ipkTop);
-    VdbeComment((v, "call rowid unique-check subroutine"));
-    sqlite3VdbeJumpHere(v, p->ipkBtm);
-    p->ipkTop = 0;
-  }
-}
-
 /*
 ** Generate code to do constraint checks prior to an INSERT or an UPDATE
 ** on table pTab.
@@ -1325,11 +1287,13 @@ void sqlite3GenerateConstraintChecks(
   int addr1;           /* Address of jump instruction */
   int seenReplace = 0; /* True if REPLACE is used to resolve INT PK conflict */
   int nPkField;        /* Number of fields in PRIMARY KEY. 1 for ROWID tables */
-  ConstraintAddr sAddr;/* Address information for constraint reordering */
   Index *pUpIdx = 0;   /* Index to which to apply the upsert */
   u8 isUpdate;         /* True if this is an UPDATE operation */
   u8 bAffinityDone = 0;  /* True if the OP_Affinity operation has been run */
   int upsertBypass = 0;  /* Address of Goto to bypass upsert subroutine */
+  int upsertJump = 0;    /* Address of Goto that jumps into upsert subroutine */
+  int ipkTop = 0;        /* Top of the IPK uniqueness check */
+  int ipkBottom = 0;     /* OP_Goto at the end of the IPK uniqueness check */
 
   isUpdate = regOldData!=0;
   db = pParse->db;
@@ -1337,7 +1301,6 @@ void sqlite3GenerateConstraintChecks(
   assert( v!=0 );
   assert( pTab->pSelect==0 );  /* This table is not a VIEW */
   nCol = pTab->nCol;
-  memset(&sAddr, 0, sizeof(sAddr));
   
   /* pPk is the PRIMARY KEY index for WITHOUT ROWID tables and NULL for
   ** normal rowid tables.  nPkField is the number of key fields in the 
@@ -1441,8 +1404,8 @@ void sqlite3GenerateConstraintChecks(
   /* UNIQUE and PRIMARY KEY constraints should be handled in the following
   ** order:
   **
-  **   (1)  OE_Abort, OE_Fail, OE_Rollback, OE_Ignore
+  **   (1)  OE_Update
-  **   (2)  OE_Update
+  **   (2)  OE_Abort, OE_Fail, OE_Rollback, OE_Ignore
   **   (3)  OE_Replace
   **
   ** OE_Fail and OE_Ignore must happen before any changes are made.
@@ -1451,6 +1414,11 @@ void sqlite3GenerateConstraintChecks(
   ** could happen in any order, but they are grouped up front for
   ** convenience.
   **
+  ** 2018-08-14: Ticket https://www.sqlite.org/src/info/908f001483982c43
+  ** The order of constraints used to have OE_Update as (2) and OE_Abort
+  ** and so forth as (1). But apparently PostgreSQL checks the OE_Update
+  ** constraint before any others, so it had to be moved.
+  **
   ** Constraint checking code is generated in this order:
   **   (A)  The rowid constraint
   **   (B)  Unique index constraints that do not have OE_Replace as their
@@ -1470,11 +1438,10 @@ void sqlite3GenerateConstraintChecks(
       overrideError = OE_Ignore;
       pUpsert = 0;
     }else if( (pUpIdx = pUpsert->pUpsertIdx)!=0 ){
-      /* If the constraint-target is on some column other than
+      /* If the constraint-target uniqueness check must be run first.
-      ** then ROWID, then we might need to move the UPSERT around
+      ** Jump to that uniqueness check now */
-      ** so that it occurs in the correct order. */
+      upsertJump = sqlite3VdbeAddOp0(v, OP_Goto);
-      sAddr.upsertTop = sAddr.upsertTop2 = sqlite3VdbeMakeLabel(v);
+      VdbeComment((v, "UPSERT constraint goes first"));
-      sAddr.upsertBtm = sqlite3VdbeMakeLabel(v);
     }
   }
 
@@ -1506,16 +1473,12 @@ void sqlite3GenerateConstraintChecks(
     ** to defer the running of the rowid conflict checking until after
     ** the UNIQUE constraints have run.
     */
-    assert( OE_Update>OE_Replace );
+    if( onError==OE_Replace      /* IPK rule is REPLACE */
-    assert( OE_Ignore<OE_Replace );
+     && onError!=overrideError   /* Rules for other contraints are different */
-    assert( OE_Fail<OE_Replace );
+     && pTab->pIndex             /* There exist other constraints */
-    assert( OE_Abort<OE_Replace );
-    assert( OE_Rollback<OE_Replace );
-    if( onError>=OE_Replace
-     && (pUpsert || onError!=overrideError)
-     && pTab->pIndex
     ){
-      sAddr.ipkTop = sqlite3VdbeAddOp0(v, OP_Goto)+1;
+      ipkTop = sqlite3VdbeAddOp0(v, OP_Goto)+1;
+      VdbeComment((v, "defer IPK REPLACE until last"));
     }
 
     if( isUpdate ){
@@ -1610,9 +1573,9 @@ void sqlite3GenerateConstraintChecks(
       }
     }
     sqlite3VdbeResolveLabel(v, addrRowidOk);
-    if( sAddr.ipkTop ){
+    if( ipkTop ){
-      sAddr.ipkBtm = sqlite3VdbeAddOp0(v, OP_Goto);
+      ipkBottom = sqlite3VdbeAddOp0(v, OP_Goto);
-      sqlite3VdbeJumpHere(v, sAddr.ipkTop-1);
+      sqlite3VdbeJumpHere(v, ipkTop-1);
     }
   }
 
@@ -1630,18 +1593,18 @@ void sqlite3GenerateConstraintChecks(
     int addrUniqueOk;    /* Jump here if the UNIQUE constraint is satisfied */
 
     if( aRegIdx[ix]==0 ) continue;  /* Skip indices that do not change */
-    if( bAffinityDone==0 ){
-      sqlite3TableAffinity(v, pTab, regNewData+1);
-      bAffinityDone = 1;
-    }
     if( pUpIdx==pIdx ){
-      addrUniqueOk = sAddr.upsertBtm;
+      addrUniqueOk = upsertJump+1;
       upsertBypass = sqlite3VdbeGoto(v, 0);
       VdbeComment((v, "Skip upsert subroutine"));
-      sqlite3VdbeResolveLabel(v, sAddr.upsertTop2);
+      sqlite3VdbeJumpHere(v, upsertJump);
     }else{
       addrUniqueOk = sqlite3VdbeMakeLabel(v);
     }
+    if( bAffinityDone==0 && (pUpIdx==0 || pUpIdx==pIdx) ){
+      sqlite3TableAffinity(v, pTab, regNewData+1);
+      bAffinityDone = 1;
+    }
     VdbeNoopComment((v, "uniqueness check for %s", pIdx->zName));
     iThisCur = iIdxCur+ix;
 
@@ -1713,15 +1676,6 @@ void sqlite3GenerateConstraintChecks(
       }
     }
 
-    /* Invoke subroutines to handle IPK replace and upsert prior to running
-    ** the first REPLACE constraint check. */
-    if( onError==OE_Replace ){
-      testcase( sAddr.ipkTop );
-      testcase( sAddr.upsertTop
-             && sqlite3VdbeLabelHasBeenResolved(v,sAddr.upsertTop) );
-      reorderConstraintChecks(v, &sAddr);
-    }
-
     /* Collision detection may be omitted if all of the following are true:
     **   (1) The conflict resolution algorithm is REPLACE
     **   (2) The table is a WITHOUT ROWID table
@@ -1843,18 +1797,21 @@ void sqlite3GenerateConstraintChecks(
       }
     }
     if( pUpIdx==pIdx ){
+      sqlite3VdbeGoto(v, upsertJump+1);
       sqlite3VdbeJumpHere(v, upsertBypass);
     }else{
       sqlite3VdbeResolveLabel(v, addrUniqueOk);
     }
     if( regR!=regIdx ) sqlite3ReleaseTempRange(pParse, regR, nPkField);
-
   }
-  testcase( sAddr.ipkTop!=0 );
+
-  testcase( sAddr.upsertTop
+  /* If the IPK constraint is a REPLACE, run it last */
-         && sqlite3VdbeLabelHasBeenResolved(v,sAddr.upsertTop) );
+  if( ipkTop ){
-  reorderConstraintChecks(v, &sAddr);
+    sqlite3VdbeGoto(v, ipkTop+1);
-  
+    VdbeComment((v, "Do IPK REPLACE"));
+    sqlite3VdbeJumpHere(v, ipkBottom);
+  }
+
   *pbMayReplace = seenReplace;
   VdbeModuleComment((v, "END: GenCnstCks(%d)", seenReplace));
 }

src/vdbe.h

@@ -238,9 +238,6 @@ void sqlite3VdbeClearObject(sqlite3*,Vdbe*);
 void sqlite3VdbeMakeReady(Vdbe*,Parse*);
 int sqlite3VdbeFinalize(Vdbe*);
 void sqlite3VdbeResolveLabel(Vdbe*, int);
-#ifdef SQLITE_COVERAGE_TEST
-  int sqlite3VdbeLabelHasBeenResolved(Vdbe*,int);
-#endif
 int sqlite3VdbeCurrentAddr(Vdbe*);
 #ifdef SQLITE_DEBUG
   int sqlite3VdbeAssertMayAbort(Vdbe *, int);

src/vdbeaux.c

@@ -437,19 +437,6 @@ void sqlite3VdbeResolveLabel(Vdbe *v, int x){
   }
 }
 
-#ifdef SQLITE_COVERAGE_TEST
-/*
-** Return TRUE if and only if the label x has already been resolved.
-** Return FALSE (zero) if label x is still unresolved.
-**
-** This routine is only used inside of testcase() macros, and so it
-** only exists when measuring test coverage.
-*/
-int sqlite3VdbeLabelHasBeenResolved(Vdbe *v, int x){
-  return v->pParse->aLabel && v->pParse->aLabel[ADDR(x)]>=0;
-}
-#endif /* SQLITE_COVERAGE_TEST */
-
 /*
 ** Mark the VDBE as one that can only be run one time.
 */

test/upsert1.test

@@ -128,4 +128,87 @@ do_execsql_test upsert1-610 {
   PRAGMA integrity_check;
 } {ok}
 
+# 2018-08-14
+# Ticket https://www.sqlite.org/src/info/908f001483982c43
+# If there are multiple uniqueness contraints, the UPSERT should fire
+# if the one constraint it targets fails, regardless of whether or not
+# the other constraints pass or fail.  In other words, the UPSERT constraint
+# should be tested first.
+#
+do_execsql_test upsert1-700 {
+  DROP TABLE t1;
+  CREATE TABLE t1(a INTEGER PRIMARY KEY, b INT, c INT, d INT, e INT);
+  CREATE UNIQUE INDEX t1b ON t1(b);
+  CREATE UNIQUE INDEX t1e ON t1(e);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,3,4,5);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,33,44,5)
+    ON CONFLICT(e) DO UPDATE SET c=excluded.c;
+  SELECT * FROM t1;
+} {1 2 33 4 5}
+do_execsql_test upsert1-710 {
+  DELETE FROM t1;
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,3,4,5);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,33,44,5)
+    ON CONFLICT(a) DO UPDATE SET c=excluded.c;
+  SELECT * FROM t1;
+} {1 2 33 4 5}
+do_execsql_test upsert1-720 {
+  DELETE FROM t1;
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,3,4,5);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,33,44,5)
+    ON CONFLICT(b) DO UPDATE SET c=excluded.c;
+  SELECT * FROM t1;
+} {1 2 33 4 5}
+do_execsql_test upsert1-730 {
+  DROP TABLE t1;
+  CREATE TABLE t1(a INT, b INT, c INT, d INT, e INT);
+  CREATE UNIQUE INDEX t1a ON t1(a);
+  CREATE UNIQUE INDEX t1b ON t1(b);
+  CREATE UNIQUE INDEX t1e ON t1(e);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,3,4,5);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,33,44,5)
+    ON CONFLICT(e) DO UPDATE SET c=excluded.c;
+  SELECT * FROM t1;
+} {1 2 33 4 5}
+do_execsql_test upsert1-740 {
+  DELETE FROM t1;
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,3,4,5);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,33,44,5)
+    ON CONFLICT(a) DO UPDATE SET c=excluded.c;
+  SELECT * FROM t1;
+} {1 2 33 4 5}
+do_execsql_test upsert1-750 {
+  DELETE FROM t1;
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,3,4,5);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,33,44,5)
+    ON CONFLICT(b) DO UPDATE SET c=excluded.c;
+  SELECT * FROM t1;
+} {1 2 33 4 5}
+do_execsql_test upsert1-760 {
+  DROP TABLE t1;
+  CREATE TABLE t1(a INT PRIMARY KEY, b INT, c INT, d INT, e INT) WITHOUT ROWID;
+  CREATE UNIQUE INDEX t1a ON t1(a);
+  CREATE UNIQUE INDEX t1b ON t1(b);
+  CREATE UNIQUE INDEX t1e ON t1(e);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,3,4,5);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,33,44,5)
+    ON CONFLICT(e) DO UPDATE SET c=excluded.c;
+  SELECT * FROM t1;
+} {1 2 33 4 5}
+do_execsql_test upsert1-770 {
+  DELETE FROM t1;
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,3,4,5);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,33,44,5)
+    ON CONFLICT(a) DO UPDATE SET c=excluded.c;
+  SELECT * FROM t1;
+} {1 2 33 4 5}
+do_execsql_test upsert1-780 {
+  DELETE FROM t1;
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,3,4,5);
+  INSERT INTO t1(a,b,c,d,e) VALUES(1,2,33,44,5)
+    ON CONFLICT(b) DO UPDATE SET c=excluded.c;
+  SELECT * FROM t1;
+} {1 2 33 4 5}
+
+
 finish_test