Index: src/insert.c ================================================================== --- src/insert.c +++ src/insert.c @@ -1176,48 +1176,10 @@ testcase( w.eCode==CKCNSTRNT_ROWID ); testcase( w.eCode==(CKCNSTRNT_ROWID|CKCNSTRNT_COLUMN) ); 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. ** ** The regNewData parameter is the first register in a range that contains @@ -1323,23 +1285,24 @@ int nCol; /* Number of columns */ int onError; /* Conflict resolution strategy */ 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; v = sqlite3GetVdbe(pParse); 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 ** pPk index or 1 for a rowid table. In other words, nPkField is the ** number of fields in the true primary key of the table. */ @@ -1439,19 +1402,24 @@ #endif /* !defined(SQLITE_OMIT_CHECK) */ /* UNIQUE and PRIMARY KEY constraints should be handled in the following ** order: ** - ** (1) OE_Abort, OE_Fail, OE_Rollback, OE_Ignore - ** (2) OE_Update + ** (1) 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. ** OE_Update guarantees that only a single row will change, so it ** must happen before OE_Replace. Technically, OE_Abort and OE_Rollback ** 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 ** default conflict resolution strategy @@ -1468,15 +1436,14 @@ ** Make all unique constraint resolution be OE_Ignore */ assert( pUpsert->pUpsertSet==0 ); overrideError = OE_Ignore; pUpsert = 0; }else if( (pUpIdx = pUpsert->pUpsertIdx)!=0 ){ - /* If the constraint-target is on some column other than - ** then ROWID, then we might need to move the UPSERT around - ** so that it occurs in the correct order. */ - sAddr.upsertTop = sAddr.upsertTop2 = sqlite3VdbeMakeLabel(v); - sAddr.upsertBtm = sqlite3VdbeMakeLabel(v); + /* If the constraint-target uniqueness check must be run first. + ** Jump to that uniqueness check now */ + upsertJump = sqlite3VdbeAddOp0(v, OP_Goto); + VdbeComment((v, "UPSERT constraint goes first")); } } /* If rowid is changing, make sure the new rowid does not previously ** exist in the table. @@ -1504,20 +1471,16 @@ /* If the response to a rowid conflict is REPLACE but the response ** to some other UNIQUE constraint is FAIL or IGNORE, then we need ** to defer the running of the rowid conflict checking until after ** the UNIQUE constraints have run. */ - assert( OE_Update>OE_Replace ); - assert( OE_Ignore=OE_Replace - && (pUpsert || onError!=overrideError) - && pTab->pIndex + if( onError==OE_Replace /* IPK rule is REPLACE */ + && onError!=overrideError /* Rules for other contraints are different */ + && pTab->pIndex /* There exist other constraints */ ){ - sAddr.ipkTop = sqlite3VdbeAddOp0(v, OP_Goto)+1; + ipkTop = sqlite3VdbeAddOp0(v, OP_Goto)+1; + VdbeComment((v, "defer IPK REPLACE until last")); } if( isUpdate ){ /* pkChng!=0 does not mean that the rowid has changed, only that ** it might have changed. Skip the conflict logic below if the rowid @@ -1608,13 +1571,13 @@ sqlite3VdbeGoto(v, ignoreDest); break; } } sqlite3VdbeResolveLabel(v, addrRowidOk); - if( sAddr.ipkTop ){ - sAddr.ipkBtm = sqlite3VdbeAddOp0(v, OP_Goto); - sqlite3VdbeJumpHere(v, sAddr.ipkTop-1); + if( ipkTop ){ + ipkBottom = sqlite3VdbeAddOp0(v, OP_Goto); + sqlite3VdbeJumpHere(v, ipkTop-1); } } /* Test all UNIQUE constraints by creating entries for each UNIQUE ** index and making sure that duplicate entries do not already exist. @@ -1628,21 +1591,21 @@ int regR; /* Range of registers holding conflicting PK */ int iThisCur; /* Cursor for this UNIQUE index */ 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; @@ -1711,19 +1674,10 @@ }else{ onError = OE_Update; /* DO UPDATE */ } } - /* 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 ** (3) There are no secondary indexes on the table ** (4) No delete triggers need to be fired if there is a conflict @@ -1841,22 +1795,25 @@ seenReplace = 1; break; } } if( pUpIdx==pIdx ){ + sqlite3VdbeGoto(v, upsertJump+1); sqlite3VdbeJumpHere(v, upsertBypass); }else{ sqlite3VdbeResolveLabel(v, addrUniqueOk); } if( regR!=regIdx ) sqlite3ReleaseTempRange(pParse, regR, nPkField); + } + /* If the IPK constraint is a REPLACE, run it last */ + if( ipkTop ){ + sqlite3VdbeGoto(v, ipkTop+1); + VdbeComment((v, "Do IPK REPLACE")); + sqlite3VdbeJumpHere(v, ipkBottom); } - testcase( sAddr.ipkTop!=0 ); - testcase( sAddr.upsertTop - && sqlite3VdbeLabelHasBeenResolved(v,sAddr.upsertTop) ); - reorderConstraintChecks(v, &sAddr); - + *pbMayReplace = seenReplace; VdbeModuleComment((v, "END: GenCnstCks(%d)", seenReplace)); } #ifdef SQLITE_ENABLE_NULL_TRIM Index: src/vdbe.h ================================================================== --- src/vdbe.h +++ src/vdbe.h @@ -236,13 +236,10 @@ void sqlite3VdbeDelete(Vdbe*); 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); #endif void sqlite3VdbeResetStepResult(Vdbe*); Index: src/vdbeaux.c ================================================================== --- src/vdbeaux.c +++ src/vdbeaux.c @@ -435,23 +435,10 @@ assert( p->aLabel[j]==(-1) ); /* Labels may only be resolved once */ p->aLabel[j] = v->nOp; } } -#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. */ void sqlite3VdbeRunOnlyOnce(Vdbe *p){ p->runOnlyOnce = 1; Index: test/upsert1.test ================================================================== --- test/upsert1.test +++ test/upsert1.test @@ -125,7 +125,90 @@ do_execsql_test upsert1-610 { DELETE FROM t1; INSERT OR IGNORE INTO t1(a) VALUES('1'),(1) ON CONFLICT(a) DO NOTHING; 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