From 1e25d20cca3ee566f38557e339c6d199772f5c96 Mon Sep 17 00:00:00 2001 From: dan Date: Sat, 20 Feb 2021 18:02:37 +0000 Subject: [PATCH] Update sqlite3changeset_apply_v2() so that it handles no-op UPDATE changes (UPDATE changes that modify no columns). This fixes a regression introduced by [e4ccfac09b]. Also modify sqlite3rebaser_rebase() so that it does not output changesets containing such UPDATEs. FossilOrigin-Name: 0288a8013e00594e716a5fb0d9f684dcfeb03e877650630e2736565fa6261290 --- ext/session/sessionnoop.test | 187 +++++++++++++++++++++++++++++++++++ ext/session/sqlite3session.c | 92 +++++++++++------ manifest | 15 +-- manifest.uuid | 2 +- 4 files changed, 259 insertions(+), 37 deletions(-) create mode 100644 ext/session/sessionnoop.test diff --git a/ext/session/sessionnoop.test b/ext/session/sessionnoop.test new file mode 100644 index 0000000000..16c60b7abf --- /dev/null +++ b/ext/session/sessionnoop.test @@ -0,0 +1,187 @@ +# 2021 Februar 20 +# +# The author disclaims copyright to this source code. In place of +# a legal notice, here is a blessing: +# +# May you do good and not evil. +# May you find forgiveness for yourself and forgive others. +# May you share freely, never taking more than you give. +# +#*********************************************************************** +# This file implements regression tests for SQLite library. +# + +if {![info exists testdir]} { + set testdir [file join [file dirname [info script]] .. .. test] +} +source [file join [file dirname [info script]] session_common.tcl] +source $testdir/tester.tcl +ifcapable !session {finish_test; return} + +set testprefix sessionnoop + +#------------------------------------------------------------------------- +# Test plan: +# +# 1.*: Test that concatenating changesets cannot produce a noop UPDATE. +# 2.*: Test that rebasing changesets cannot produce a noop UPDATE. +# 3.*: Test that sqlite3changeset_apply() ignores noop UPDATE changes. +# + +do_execsql_test 1.0 { + CREATE TABLE t1(a PRIMARY KEY, b, c, d); + INSERT INTO t1 VALUES(1, 1, 1, 1); + INSERT INTO t1 VALUES(2, 2, 2, 2); + INSERT INTO t1 VALUES(3, 3, 3, 3); +} + +proc do_concat_test {tn sql1 sql2 res} { + uplevel [list do_test $tn [subst -nocommands { + set C1 [changeset_from_sql {$sql1}] + set C2 [changeset_from_sql {$sql2}] + set C3 [sqlite3changeset_concat [set C1] [set C2]] + set got [list] + sqlite3session_foreach elem [set C3] { lappend got [set elem] } + set got + }] [list {*}$res]] +} + +do_concat_test 1.1 { + UPDATE t1 SET c=c+1; +} { + UPDATE t1 SET c=c-1; +} { +} + +#------------------------------------------------------------------------- +reset_db +do_execsql_test 2.0 { + CREATE TABLE t1(a PRIMARY KEY, b, c); + INSERT INTO t1 VALUES(1, 1, 1); + INSERT INTO t1 VALUES(2, 2, 2); + INSERT INTO t1 VALUES(3, 3, 3); +} + +proc do_rebase_test {tn sql_local sql_remote conflict_res expected} { + proc xConflict {args} [list return $conflict_res] + + uplevel [list \ + do_test $tn [subst -nocommands { + execsql BEGIN + set c_remote [changeset_from_sql {$sql_remote}] + execsql ROLLBACK + + execsql BEGIN + set c_local [changeset_from_sql {$sql_local}] + set base [sqlite3changeset_apply_v2 db [set c_remote] xConflict] + execsql ROLLBACK + + sqlite3rebaser_create R + R config [set base] + set res [list] + sqlite3session_foreach elem [R rebase [set c_local]] { + lappend res [set elem] + } + R delete + set res + }] [list {*}$expected] + ] +} + +do_rebase_test 2.1 { + UPDATE t1 SET c=2 WHERE a=1; -- local +} { + UPDATE t1 SET c=3 WHERE a=1; -- remote +} OMIT { + {UPDATE t1 0 X.. {i 1 {} {} i 3} {{} {} {} {} i 2}} +} + +do_rebase_test 2.2 { + UPDATE t1 SET c=2 WHERE a=1; -- local +} { + UPDATE t1 SET c=3 WHERE a=1; -- remote +} REPLACE { +} + +do_rebase_test 2.3.1 { + UPDATE t1 SET c=4 WHERE a=1; -- local +} { + UPDATE t1 SET c=4 WHERE a=1 -- remote +} OMIT { + {UPDATE t1 0 X.. {i 1 {} {} i 4} {{} {} {} {} i 4}} +} + +do_rebase_test 2.3.2 { + UPDATE t1 SET c=5 WHERE a=1; -- local +} { + UPDATE t1 SET c=5 WHERE a=1 -- remote +} REPLACE { +} + +#------------------------------------------------------------------------- +# +reset_db +do_execsql_test 3.0 { + CREATE TABLE t1(a INTEGER PRIMARY KEY, b, c); + INSERT INTO t1 VALUES(1, 1, 1); + INSERT INTO t1 VALUES(2, 2, 2); + INSERT INTO t1 VALUES(3, 3, 3); + INSERT INTO t1 VALUES(4, 4, 4); +} + +# Arg $pkstr contains one character for each column in the table. An +# "X" for PK column, or a "." for a non-PK. +# +proc mk_tbl_header {name pkstr} { + set ret [binary format H2c 54 [string length $pkstr]] + foreach i [split $pkstr {}] { + if {$i=="X"} { + append ret [binary format H2 01] + } else { + if {$i!="."} {error "bad pkstr: $pkstr ($i)"} + append ret [binary format H2 00] + } + } + append ret $name + append ret [binary format H2 00] + set ret +} + +proc mk_update_change {args} { + set ret [binary format H2H2 17 00] + foreach a $args { + if {$a==""} { + append ret [binary format H2 00] + } else { + append ret [binary format H2W 01 $a] + } + } + set ret +} + +proc xConflict {args} { return "ABORT" } +do_test 3.1 { + set C [mk_tbl_header t1 X..] + append C [mk_update_change 1 {} 1 {} {} 500] + append C [mk_update_change 2 {} {} {} {} {}] + append C [mk_update_change 3 3 {} {} 600 {}] + append C [mk_update_change 4 {} {} {} {} {}] + + sqlite3changeset_apply_v2 db $C xConflict +} {} +do_execsql_test 3.2 { + SELECT * FROM t1 +} { + 1 1 500 + 2 2 2 + 3 600 3 + 4 4 4 +} + + + + + + +finish_test + diff --git a/ext/session/sqlite3session.c b/ext/session/sqlite3session.c index cafa5b29b7..8210056a89 100644 --- a/ext/session/sqlite3session.c +++ b/ext/session/sqlite3session.c @@ -91,6 +91,7 @@ struct sqlite3_changeset_iter { SessionBuffer tblhdr; /* Buffer to hold apValue/zTab/abPK/ */ int bPatchset; /* True if this is a patchset */ int bInvert; /* True to invert changeset */ + int bSkipEmpty; /* Skip noop UPDATE changes */ int rc; /* Iterator error code */ sqlite3_stmt *pConflict; /* Points to conflicting row, if any */ char *zTab; /* Current table */ @@ -2620,7 +2621,8 @@ static int sessionChangesetStart( void *pIn, int nChangeset, /* Size of buffer pChangeset in bytes */ void *pChangeset, /* Pointer to buffer containing changeset */ - int bInvert /* True to invert changeset */ + int bInvert, /* True to invert changeset */ + int bSkipEmpty /* True to skip empty UPDATE changes */ ){ sqlite3_changeset_iter *pRet; /* Iterator to return */ int nByte; /* Number of bytes to allocate for iterator */ @@ -2641,6 +2643,7 @@ static int sessionChangesetStart( pRet->in.pIn = pIn; pRet->in.bEof = (xInput ? 0 : 1); pRet->bInvert = bInvert; + pRet->bSkipEmpty = bSkipEmpty; /* Populate the output variable and return success. */ *pp = pRet; @@ -2655,7 +2658,7 @@ int sqlite3changeset_start( int nChangeset, /* Size of buffer pChangeset in bytes */ void *pChangeset /* Pointer to buffer containing changeset */ ){ - return sessionChangesetStart(pp, 0, 0, nChangeset, pChangeset, 0); + return sessionChangesetStart(pp, 0, 0, nChangeset, pChangeset, 0, 0); } int sqlite3changeset_start_v2( sqlite3_changeset_iter **pp, /* OUT: Changeset iterator handle */ @@ -2664,7 +2667,7 @@ int sqlite3changeset_start_v2( int flags ){ int bInvert = !!(flags & SQLITE_CHANGESETSTART_INVERT); - return sessionChangesetStart(pp, 0, 0, nChangeset, pChangeset, bInvert); + return sessionChangesetStart(pp, 0, 0, nChangeset, pChangeset, bInvert, 0); } /* @@ -2675,7 +2678,7 @@ int sqlite3changeset_start_strm( int (*xInput)(void *pIn, void *pData, int *pnData), void *pIn ){ - return sessionChangesetStart(pp, xInput, pIn, 0, 0, 0); + return sessionChangesetStart(pp, xInput, pIn, 0, 0, 0, 0); } int sqlite3changeset_start_v2_strm( sqlite3_changeset_iter **pp, /* OUT: Changeset iterator handle */ @@ -2684,7 +2687,7 @@ int sqlite3changeset_start_v2_strm( int flags ){ int bInvert = !!(flags & SQLITE_CHANGESETSTART_INVERT); - return sessionChangesetStart(pp, xInput, pIn, 0, 0, bInvert); + return sessionChangesetStart(pp, xInput, pIn, 0, 0, bInvert, 0); } /* @@ -2810,11 +2813,14 @@ static int sessionReadRecord( SessionInput *pIn, /* Input data */ int nCol, /* Number of values in record */ u8 *abPK, /* Array of primary key flags, or NULL */ - sqlite3_value **apOut /* Write values to this array */ + sqlite3_value **apOut, /* Write values to this array */ + int *pbEmpty ){ int i; /* Used to iterate through columns */ int rc = SQLITE_OK; + assert( pbEmpty==0 || *pbEmpty==0 ); + if( pbEmpty ) *pbEmpty = 1; for(i=0; iaData[pIn->iNext++]; assert( apOut[i]==0 ); if( eType ){ + if( pbEmpty ) *pbEmpty = 0; apOut[i] = sqlite3ValueNew(0); if( !apOut[i] ) rc = SQLITE_NOMEM; } @@ -3005,31 +3012,27 @@ static int sessionChangesetReadTblhdr(sqlite3_changeset_iter *p){ } /* -** Advance the changeset iterator to the next change. +** Advance the changeset iterator to the next change. The differences between +** this function and sessionChangesetNext() are that ** -** If both paRec and pnRec are NULL, then this function works like the public -** API sqlite3changeset_next(). If SQLITE_ROW is returned, then the -** sqlite3changeset_new() and old() APIs may be used to query for values. +** * If pbEmpty is not NULL and the change is a no-op UPDATE (an UPDATE +** that modifies no columns), this function sets (*pbEmpty) to 1. ** -** Otherwise, if paRec and pnRec are not NULL, then a pointer to the change -** record is written to *paRec before returning and the number of bytes in -** the record to *pnRec. -** -** Either way, this function returns SQLITE_ROW if the iterator is -** successfully advanced to the next change in the changeset, an SQLite -** error code if an error occurs, or SQLITE_DONE if there are no further -** changes in the changeset. +** * If the iterator is configured to skip no-op UPDATEs, +** sessionChangesetNext() does that. This function does not. */ -static int sessionChangesetNext( +static int sessionChangesetNextOne( sqlite3_changeset_iter *p, /* Changeset iterator */ u8 **paRec, /* If non-NULL, store record pointer here */ int *pnRec, /* If non-NULL, store size of record here */ - int *pbNew /* If non-NULL, true if new table */ + int *pbNew, /* If non-NULL, true if new table */ + int *pbEmpty ){ int i; u8 op; assert( (paRec==0 && pnRec==0) || (paRec && pnRec) ); + assert( pbEmpty==0 || *pbEmpty==0 ); /* If the iterator is in the error-state, return immediately. */ if( p->rc!=SQLITE_OK ) return p->rc; @@ -3102,13 +3105,13 @@ static int sessionChangesetNext( /* If this is an UPDATE or DELETE, read the old.* record. */ if( p->op!=SQLITE_INSERT && (p->bPatchset==0 || p->op==SQLITE_DELETE) ){ u8 *abPK = p->bPatchset ? p->abPK : 0; - p->rc = sessionReadRecord(&p->in, p->nCol, abPK, apOld); + p->rc = sessionReadRecord(&p->in, p->nCol, abPK, apOld, 0); if( p->rc!=SQLITE_OK ) return p->rc; } /* If this is an INSERT or UPDATE, read the new.* record. */ if( p->op!=SQLITE_DELETE ){ - p->rc = sessionReadRecord(&p->in, p->nCol, 0, apNew); + p->rc = sessionReadRecord(&p->in, p->nCol, 0, apNew, pbEmpty); if( p->rc!=SQLITE_OK ) return p->rc; } @@ -3135,6 +3138,37 @@ static int sessionChangesetNext( return SQLITE_ROW; } +/* +** Advance the changeset iterator to the next change. +** +** If both paRec and pnRec are NULL, then this function works like the public +** API sqlite3changeset_next(). If SQLITE_ROW is returned, then the +** sqlite3changeset_new() and old() APIs may be used to query for values. +** +** Otherwise, if paRec and pnRec are not NULL, then a pointer to the change +** record is written to *paRec before returning and the number of bytes in +** the record to *pnRec. +** +** Either way, this function returns SQLITE_ROW if the iterator is +** successfully advanced to the next change in the changeset, an SQLite +** error code if an error occurs, or SQLITE_DONE if there are no further +** changes in the changeset. +*/ +static int sessionChangesetNext( + sqlite3_changeset_iter *p, /* Changeset iterator */ + u8 **paRec, /* If non-NULL, store record pointer here */ + int *pnRec, /* If non-NULL, store size of record here */ + int *pbNew /* If non-NULL, true if new table */ +){ + int bEmpty; + int rc; + do { + bEmpty = 0; + rc = sessionChangesetNextOne(p, paRec, pnRec, pbNew, &bEmpty); + }while( rc==SQLITE_ROW && p->bSkipEmpty && bEmpty); + return rc; +} + /* ** Advance an iterator created by sqlite3changeset_start() to the next ** change in the changeset. This function may return SQLITE_ROW, SQLITE_DONE @@ -3407,9 +3441,9 @@ static int sessionChangesetInvert( /* Read the old.* and new.* records for the update change. */ pInput->iNext += 2; - rc = sessionReadRecord(pInput, nCol, 0, &apVal[0]); + rc = sessionReadRecord(pInput, nCol, 0, &apVal[0], 0); if( rc==SQLITE_OK ){ - rc = sessionReadRecord(pInput, nCol, 0, &apVal[nCol]); + rc = sessionReadRecord(pInput, nCol, 0, &apVal[nCol], 0); } /* Write the new old.* record. Consists of the PK columns from the @@ -4357,7 +4391,7 @@ static int sessionRetryConstraints( memset(&pApply->constraints, 0, sizeof(SessionBuffer)); rc = sessionChangesetStart( - &pIter2, 0, 0, cons.nBuf, cons.aBuf, pApply->bInvertConstraints + &pIter2, 0, 0, cons.nBuf, cons.aBuf, pApply->bInvertConstraints, 1 ); if( rc==SQLITE_OK ){ size_t nByte = 2*pApply->nCol*sizeof(sqlite3_value*); @@ -4613,8 +4647,8 @@ int sqlite3changeset_apply_v2( int flags ){ sqlite3_changeset_iter *pIter; /* Iterator to skip through changeset */ - int bInverse = !!(flags & SQLITE_CHANGESETAPPLY_INVERT); - int rc = sessionChangesetStart(&pIter, 0, 0, nChangeset, pChangeset,bInverse); + int bInv = !!(flags & SQLITE_CHANGESETAPPLY_INVERT); + int rc = sessionChangesetStart(&pIter, 0, 0, nChangeset, pChangeset, bInv, 1); if( rc==SQLITE_OK ){ rc = sessionChangesetApply( db, pIter, xFilter, xConflict, pCtx, ppRebase, pnRebase, flags @@ -4672,7 +4706,7 @@ int sqlite3changeset_apply_v2_strm( ){ sqlite3_changeset_iter *pIter; /* Iterator to skip through changeset */ int bInverse = !!(flags & SQLITE_CHANGESETAPPLY_INVERT); - int rc = sessionChangesetStart(&pIter, xInput, pIn, 0, 0, bInverse); + int rc = sessionChangesetStart(&pIter, xInput, pIn, 0, 0, bInverse, 1); if( rc==SQLITE_OK ){ rc = sessionChangesetApply( db, pIter, xFilter, xConflict, pCtx, ppRebase, pnRebase, flags @@ -5292,7 +5326,7 @@ static void sessionAppendPartialUpdate( int n1 = sessionSerialLen(a1); int n2 = sessionSerialLen(a2); if( pIter->abPK[i] || a2[0]==0 ){ - if( !pIter->abPK[i] ) bData = 1; + if( !pIter->abPK[i] && a1[0] ) bData = 1; memcpy(pOut, a1, n1); pOut += n1; }else if( a2[0]!=0xFF ){ diff --git a/manifest b/manifest index f6ba8bff30..2a87f6682f 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Break\sout\sthe\sCte\sobject\sfrom\sthe\sWith\sobject.\s\sThis\swill\smake\sit\ssimpler\nto\sadd\snew\skinds\sof\sCte\sobjects\s(ex:\sDML\sstatements)\sand/or\sMATERIALIZED\nkeywords\sin\sthe\sfuture.\s\sIt\sbrings\strunk\sinto\scloser\salignment\swith\sthe\nexperimental\sas-materialize\sbranch. -D 2021-02-20T14:57:16.222 +C Update\ssqlite3changeset_apply_v2()\sso\sthat\sit\shandles\sno-op\sUPDATE\schanges\s(UPDATE\schanges\sthat\smodify\sno\scolumns).\sThis\sfixes\sa\sregression\sintroduced\sby\s[e4ccfac09b].\sAlso\smodify\ssqlite3rebaser_rebase()\sso\sthat\sit\sdoes\snot\soutput\schangesets\scontaining\ssuch\sUPDATEs. +D 2021-02-20T18:02:37.933 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724 @@ -451,10 +451,11 @@ F ext/session/sessionfault.test da273f2712b6411e85e71465a1733b8501dbf6f7 F ext/session/sessionfault2.test dd593f80b6b4786f7adfe83c5939620bc505559770cc181332da26f29cddd7bb F ext/session/sessioninvert.test 04075517a9497a80d39c495ba6b44f3982c7371129b89e2c52219819bc105a25 F ext/session/sessionmem.test f2a735db84a3e9e19f571033b725b0b2daf847f3f28b1da55a0c1a4e74f1de09 +F ext/session/sessionnoop.test a9366a36a95ef85f8a3687856ebef46983df399541174cb1ede2ee53b8011bc7 F ext/session/sessionrebase.test ccfa716b23bd1d3b03217ee58cfd90c78d4b99f53e6a9a2f05e82363b9142810 F ext/session/sessionstat1.test 218d351cf9fcd6648f125a26b607b140310160184723c2666091b54450a68fb5 F ext/session/sessionwor.test 6fd9a2256442cebde5b2284936ae9e0d54bde692d0f5fd009ecef8511f4cf3fc -F ext/session/sqlite3session.c 1d0553077b55ffcfa69963c354e9bad3bace6ce79bbe7368e650c6ae1e106314 +F ext/session/sqlite3session.c a7c5ac1acfe21d94b37921b29b0458d64d022a66b282338eee4aafa9c018cb1c F ext/session/sqlite3session.h f53c99731882bf59c7362855cdeba176ce1fe8eeba089e38a8cce0172f8473aa F ext/session/test_session.c 93ca965112d2b4d9d669c9c0be6b1e52942a268796050a145612df1eee175ce0 F ext/userauth/sqlite3userauth.h 7f3ea8c4686db8e40b0a0e7a8e0b00fac13aa7a3 @@ -1904,7 +1905,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 03805a6117c813a33f9bca68bf4d9912997d6abd88ac9b3cb844c5d93ec68049 -R a0c1971a7f0c50b4334b5a0eaa224033 -U drh -Z 4aeed9ebd4e584b2ba4b31a4e09673dd +P f03efe905d7b40fb25f9f78b874bb56c6d6ccacb60f86b3b199d430d5eade8d2 +R abe6f39b4d6e21d7c13ff311c0c06b1f +U dan +Z af5325b22c1dec3dd9459f9acd3cf21e diff --git a/manifest.uuid b/manifest.uuid index 5ec135f568..110e522406 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -f03efe905d7b40fb25f9f78b874bb56c6d6ccacb60f86b3b199d430d5eade8d2 \ No newline at end of file +0288a8013e00594e716a5fb0d9f684dcfeb03e877650630e2736565fa6261290 \ No newline at end of file