From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Alex Khatskevich <avkhatskevich@tarantool.org>, Kirill Yukhin <kyukhin@tarantool.org> Subject: [tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes Date: Thu, 18 Oct 2018 17:41:03 +0300 [thread overview] Message-ID: <828A30A0-135C-42C7-A810-230FC227C7EA@tarantool.org> (raw) In-Reply-To: <72b1a158-ef2d-50f6-2ad5-5ffcf598b4c1@tarantool.org> Unfortunately, I’ve figured out that it still shows incorrect behaviour: box.sql.execute('create table t1 (id int primary key);') box.sql.execute('create table t2 (id int primary key);') box.sql.execute('CREATE TRIGGER TRIG1 AFTER INSERT ON T1 FOR EACH ROW BEGIN insert into t2 values(new.id); end;') box.sql.execute('select total_changes()') --- - - [3] ... box.sql.execute("start transaction") box.sql.execute("insert into t1 values(5), (6)"); box.sql.execute("rollback”) tarantool> box.sql.execute('select total_changes()') --- - - [5] … Total changes counter has increased, but both tables are empty. Since changes to data occurred by triggers are not accounted to simple changes() function, I suggest to do the same with total_changes(). I also removed vdbe_changes_update() function, instead I inlined all changes of changes-counter and added explaining comments. Overall, I assume we’ve spent enough time for such insignificant feature. My branch is https://github.com/tarantool/tarantool/tree/np/gh-2181-rework-change-counter Reworked patch is below, I am done with it. Kirill, now it is up to you: sql: increase total_changes only on commit This commit repairs total_changes() behaviour. Before this patch, on transaction (or savepoint) rollback total_changes counter wasn't returned back to its previous value. Now total_changes counter is increased only on transaction's commit. Also, now total_changes doesn't include changes made by triggers (i.e. behaves in the same way as changes() does). Closes #2181 @TarantoolBot document Title: SQL changes() and total_changes() behavior changes() and total_changes() functions work almost similarly to SQLite. `changes`: * shows number of changed by the last statement rows * updated after each statement * doesn't include changes made by triggers `total_changes`: * shows number of changed rows since db was started * updated on commit (or auto-commit) * also doesn't include changes made by triggers Notes: * Changes which are counted: DELETE, UPDATE, INSERT. * Successful DDL requests are accounted as one change. * Changes made outside of SQL are not counted. Usage example: ``` <<db started>> <<statement: delete 1 row>> select changes(); -- return 1 <<statement: insert 2 rows>> select changes(); -- returns 2 select total_changes(); -- returns 3 ``` diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 744b66072..f56fdca25 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1758,6 +1758,8 @@ struct Savepoint { box_txn_savepoint_t *tnt_savepoint; /* Tarantool's savepoint struct */ char *zName; /* Savepoint name (nul-terminated) */ Savepoint *pNext; /* Parent savepoint (if any) */ + /** Changes to db made since last START TRANSACTION. */ + uint32_t change_count; }; /* @@ -4301,7 +4303,6 @@ Expr *sqlite3ExprAddCollateToken(Parse * pParse, Expr *, const Token *, int); Expr *sqlite3ExprAddCollateString(Parse *, Expr *, const char *); Expr *sqlite3ExprSkipCollate(Expr *); int sqlite3CheckIdentifierName(Parse *, char *); -void sqlite3VdbeSetChanges(sqlite3 *, int); int sqlite3AddInt64(i64 *, i64); int sqlite3SubInt64(i64 *, i64); int sqlite3MulInt64(i64 *, i64); diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 7c1015cf9..df5cf9682 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -988,7 +988,6 @@ case OP_Halt: { pFrame = p->pFrame; p->pFrame = pFrame->pParent; p->nFrame--; - sqlite3VdbeSetChanges(db, p->nChange); pcx = sqlite3VdbeFrameRestore(pFrame); if (pOp->p2 == ON_CONFLICT_ACTION_IGNORE) { /* Instruction pcx is the OP_Program that invoked the sub-program @@ -2881,7 +2880,7 @@ case OP_Savepoint: { if (p1==SAVEPOINT_BEGIN) { /* Create a new savepoint structure. */ - pNew = sql_savepoint(p, zName); + pNew = sql_savepoint_create(zName, psql_txn->change_count); /* Link the new savepoint into the database handle's list. */ pNew->pNext = psql_txn->pSavepoint; psql_txn->pSavepoint = pNew; @@ -2930,6 +2929,17 @@ case OP_Savepoint: { * have to destroy them */ } + if (p1 == SAVEPOINT_ROLLBACK) { + /* + * On savepoint rollback we should + * discard all changes made after + * it. + */ + assert(psql_txn->pSavepoint != NULL); + psql_txn->change_count = + psql_txn->pSavepoint->change_count; + p->nChange = 0; + } /* If it is a RELEASE, then destroy the savepoint being operated on * too. If it is a ROLLBACK TO, then set the number of deferred @@ -3000,6 +3010,18 @@ case OP_TransactionCommit: { rc = SQL_TARANTOOL_ERROR; goto abort_due_to_error; } + /* + * On commit we should account all changes made + * in current transaction i.e. changes occurred + * within processing current statement plus + * changes happen in transaction. Moreover, we + * nullify local changes since <COMMIT> statement + * doesn't really provide any changes, it only + * accounts already made changes. + */ + assert(p->psql_txn != NULL); + db->nTotalChange += p->psql_txn->change_count + p->nChange; + db->nChange = 0; } else { sqlite3VdbeError(p, "cannot commit - no transaction is active"); rc = SQLITE_ERROR; @@ -3019,6 +3041,19 @@ case OP_TransactionRollback: { rc = SQL_TARANTOOL_ERROR; goto abort_due_to_error; } + /* + * On rollback all changed made in current + * transaction should not be accounted. + * We don't care about local VDBE change + * counter since it will be wasted anyway: + * OP_TransactionRollback usually is the only + * (meaningful) opcode in program which implements + * SQL statement <START TRANSACTION>. We also + * don't care about total changes counter + * because it is incremented only on transaction + * commit or at the end of VDBE execution. + */ + db->nChange = 0; } else { sqlite3VdbeError(p, "cannot rollback - no " "transaction is active"); @@ -3045,7 +3080,10 @@ case OP_TTransaction: { goto abort_due_to_error; } } else { - p->anonymous_savepoint = sql_savepoint(p, NULL); + assert(p->psql_txn != NULL); + uint32_t change_count = p->psql_txn->change_count; + p->anonymous_savepoint = sql_savepoint_create(NULL, + change_count); if (p->anonymous_savepoint == NULL) { rc = SQL_TARANTOOL_ERROR; goto abort_due_to_error; @@ -3806,15 +3844,13 @@ case OP_Delete: { break; } + /* Opcode: ResetCount * * * * * * - * The value of the change counter is copied to the database handle - * change counter (returned by subsequent calls to sqlite3_changes()). - * Then the VMs internal change counter resets to 0. + * This routine resets VMs internal change counter to 0. * This is used by trigger programs. */ case OP_ResetCount: { - sqlite3VdbeSetChanges(db, p->nChange); p->nChange = 0; p->ignoreRaised = 0; break; @@ -4266,8 +4302,6 @@ case OP_IdxReplace: case OP_IdxInsert: { pIn2 = &aMem[pOp->p1]; assert((pIn2->flags & MEM_Blob) != 0); - if (pOp->p5 & OPFLAG_NCHANGE) - p->nChange++; rc = ExpandBlob(pIn2); if (rc != 0) goto abort_due_to_error; @@ -4293,6 +4327,9 @@ case OP_IdxInsert: { } if (pOp->p5 & OPFLAG_OE_IGNORE) { + /* Compensate nChange increment. */ + if (rc != SQLITE_OK) + p->nChange--; /* Ignore any kind of failes and do not raise error message */ rc = SQLITE_OK; /* If we are in trigger, increment ignore raised counter */ @@ -4305,6 +4342,8 @@ case OP_IdxInsert: { } if (rc != 0) goto abort_due_to_error; + if (pOp->p5 & OPFLAG_NCHANGE) + p->nChange++; break; } diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index ce97f4984..8172cd0eb 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -440,9 +440,10 @@ int sqlite3VdbeExec(Vdbe *); int sqlite3VdbeList(Vdbe *); int sql_txn_begin(Vdbe *p); -Savepoint * -sql_savepoint(Vdbe *p, - const char *zName); + +struct Savepoint * +sql_savepoint_create(const char *name, uint32_t change_count); + int sqlite3VdbeHalt(Vdbe *); int sqlite3VdbeMemTooBig(Mem *); int sqlite3VdbeMemCopy(Mem *, const Mem *); diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 6e42d0596..6a0bd7b06 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -85,6 +85,7 @@ sql_alloc_txn() return NULL; } txn->pSavepoint = NULL; + txn->change_count = 0; txn->fk_deferred_count = 0; return txn; } @@ -2270,30 +2271,27 @@ sql_txn_begin(Vdbe *p) return 0; } -Savepoint * -sql_savepoint(MAYBE_UNUSED Vdbe *p, const char *zName) +struct Savepoint * +sql_savepoint_create(const char *name, uint32_t change_count) { - assert(p); - size_t nName = zName ? strlen(zName) + 1 : 0; - size_t savepoint_sz = sizeof(Savepoint) + nName; - Savepoint *pNew; - - pNew = (Savepoint *)region_aligned_alloc(&fiber()->gc, - savepoint_sz, - alignof(Savepoint)); - if (pNew == NULL) { - diag_set(OutOfMemory, savepoint_sz, "region", - "savepoint"); + size_t name_len = name != NULL ? strlen(name) + 1 : 0; + size_t savepoint_sz = sizeof(struct Savepoint) + name_len; + struct Savepoint *savepoint = + (struct Savepoint *) region_alloc(&fiber()->gc, savepoint_sz); + if (savepoint == NULL) { + diag_set(OutOfMemory, savepoint_sz, "region", "savepoint"); return NULL; } - pNew->tnt_savepoint = box_txn_savepoint(); - if (!pNew->tnt_savepoint) + savepoint->tnt_savepoint = box_txn_savepoint(); + if (savepoint->tnt_savepoint == NULL) return NULL; - if (zName) { - pNew->zName = (char *)&pNew[1]; - memcpy(pNew->zName, zName, nName); - }; - return pNew; + savepoint->change_count = change_count; + if (name != NULL) { + /* Name is placed right after structure itself. */ + savepoint->zName = (char *) &savepoint[1]; + memcpy(savepoint->zName, name, name_len); + } + return savepoint; } /* @@ -2472,18 +2470,25 @@ sqlite3VdbeHalt(Vdbe * p) p->nChange = 0; } } - - /* If this was an INSERT, UPDATE or DELETE and no statement transaction - * has been rolled back, update the database connection change-counter. + /* + * Update the database change-counter on DML + * requests. Total changes counter is updated only + * at the end of committed transaction or in + * auto-commit mode. Otherwise, we transfer local + * counter to the next VDBE. Also, we don't need + * to even try to calculate change counter on + * EXPLAIN queries. */ - if (p->changeCntOn) { - if (eStatementOp != SAVEPOINT_ROLLBACK) { - sqlite3VdbeSetChanges(db, p->nChange); + if (p->changeCntOn && !p->explain) { + db->nChange = p->nChange; + if (p->auto_commit) { + db->nTotalChange += p->nChange; } else { - sqlite3VdbeSetChanges(db, 0); + assert(p->psql_txn != NULL); + p->psql_txn->change_count += p->nChange; } - p->nChange = 0; } + p->nChange = 0; } closeCursorsAndFree(p); @@ -3439,17 +3444,6 @@ sqlite3MemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pCol return sqlite3BlobCompare(pMem1, pMem2); } -/* - * This routine sets the value to be returned by subsequent calls to - * sqlite3_changes() on the database handle 'db'. - */ -void -sqlite3VdbeSetChanges(sqlite3 * db, int nChange) -{ - db->nChange = nChange; - db->nTotalChange += nChange; -} - /* * Set a flag in the vdbe to update the change counter when it is finalised * or reset. diff --git a/src/box/txn.h b/src/box/txn.h index e74c14d40..7d9fbbd51 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -112,6 +112,12 @@ extern double too_long_threshold; struct sql_txn { /** List of active SQL savepoints. */ struct Savepoint *pSavepoint; + /** + * Changes have been made in current transaction. + * It is needed to handle outer SQL function + * total_changes(). + */ + uint32_t change_count; /** * This variables transfer deferred constraints from one * VDBE to the next in the same transaction. diff --git a/test/sql-tap/gh2181-changes-statistics.test.lua b/test/sql-tap/gh2181-changes-statistics.test.lua new file mode 100755 index 000000000..fec6627ea --- /dev/null +++ b/test/sql-tap/gh2181-changes-statistics.test.lua @@ -0,0 +1,124 @@ +#!/usr/bin/env tarantool +test = require("sqltester") +test:plan(79) + +test:do_select_tests( + "gh2181-changes-statistics", + { + {"0.1", "SELECT CHANGES()", {0}}, + {"0.2", "SELECT TOTAL_CHANGES()", {0}}, + {"1.0", "CREATE TABLE T1(A PRIMARY KEY)", {}}, + {"1.1", "SELECT CHANGES()", {1}}, + {"1.2", "SELECT TOTAL_CHANGES()", {1}}, + {"2.0", "INSERT INTO T1 VALUES(1)", {}}, + {"2.1", "SELECT CHANGES()", {1}}, + {"2.2", "SELECT TOTAL_CHANGES()", {2}}, + {"3.0", "INSERT INTO T1 VALUES(2)", {}}, + {"3.1", "SELECT CHANGES()", {1}}, + {"3.2", "SELECT TOTAL_CHANGES()", {3}}, + {"4.0", "CREATE TABLE T2(A PRIMARY KEY)", {}}, + {"4.1", "SELECT CHANGES()", {1}}, + {"4.2", "SELECT TOTAL_CHANGES()", {4}}, + {"5.0", "INSERT INTO T2 SELECT * FROM T1", {}}, + {"5.1", "SELECT CHANGES()", {2}}, + {"5.2", "SELECT TOTAL_CHANGES()", {6}}, + {"6.0", [[ + CREATE TRIGGER TRIG1 AFTER INSERT ON T1 FOR EACH ROW BEGIN + INSERT INTO T2 VALUES(NEW.A); + END; + ]],{}}, + {"6.1", "SELECT CHANGES()", {1}}, + {"6.2", "SELECT TOTAL_CHANGES()", {7}}, + {"6.3", "INSERT INTO T1 VALUES(3)", {}}, + {"6.4", "SELECT CHANGES()", {1}}, + {"6.5", "SELECT TOTAL_CHANGES()", {8}}, + {"6.6", "DROP TRIGGER TRIG1"}, + {"6.7", "SELECT CHANGES()", {1}}, + {"6.8", "SELECT TOTAL_CHANGES()", {9}}, + {"7.0", "DELETE FROM T1 WHERE A = 1", {}}, + {"7.1", "SELECT CHANGES()", {1}}, + {"7.2", "SELECT TOTAL_CHANGES()", {10}}, + {"8.0", "UPDATE T1 SET A = 1 where A = 2", {}}, + {"8.1", "SELECT CHANGES()", {1}}, + {"8.2", "SELECT TOTAL_CHANGES()", {11}}, + {"9.0", "SELECT COUNT(*) FROM T2", {3}}, + {"9.1", "UPDATE T2 SET A = A + 3", {}}, + -- Inserts to an ephemeral space are not counted. + {"9.2", "SELECT CHANGES()", {3}}, + {"9.3", "SELECT TOTAL_CHANGES()", {14}}, + {"11.0", "DELETE FROM T1", {}}, + {"11.1", "SELECT CHANGES()", {0}}, + {"11.2", "SELECT TOTAL_CHANGES()", {14}}, + {"12.0", "DELETE FROM T2 WHERE A < 100", {}}, + {"12.1", "SELECT CHANGES()", {3}}, + {"12.2", "SELECT TOTAL_CHANGES()", {17}}, + -- Transactions/savepoints. + {"13.0", "START TRANSACTION", {}}, + {"13.1", "INSERT INTO T1 VALUES(11)", {}}, + {"13.2", "SELECT CHANGES()", {1}}, + {"13.3", "SELECT TOTAL_CHANGES()", {17}}, + {"13.4", "SAVEPOINT S1", {}}, + {"13.5", "INSERT INTO T1 VALUES(12)", {}}, + {"13.6", "SELECT CHANGES()", {1}}, + {"13.7", "SELECT TOTAL_CHANGES()", {17}}, + {"13.8", "ROLLBACK TO SAVEPOINT S1", {}}, + {"13.9", "SELECT CHANGES()", {1}}, + {"13.10", "SELECT TOTAL_CHANGES()", {17}}, + {"13.11", "COMMIT", {}}, + {"13.12", "SELECT CHANGES()", {0}}, + {"13.13", "SELECT TOTAL_CHANGES()", {18}}, + {"14.0", "START TRANSACTION", {}}, + {"14.1", "INSERT INTO T1 VALUES(15)", {}}, + {"14.2", "SELECT CHANGES()", {1}}, + {"14.3", "SELECT TOTAL_CHANGES()", {18}}, + {"14.4", "ROLLBACK", {}}, + {"14.5", "SELECT CHANGES()", {0}}, + {"14.6", "SELECT TOTAL_CHANGES()", {18}}, + {"15.0.1", "INSERT INTO T1 VALUES(1)", {}}, + {"15.0.2", "SELECT TOTAL_CHANGES()", {19}}, + }) + +test:do_catchsql_test( + "gh2181-changes-statistics-15.1", + -- Rollback during autocommit. + "INSERT INTO T1 VALUES(0),(1)", + {1, "Duplicate key exists in unique index 'pk_unnamed_T1_1' in space 'T1'"}) + +test:do_select_tests( + "gh2181-changes-statistics", + { + {"15.2", "SELECT CHANGES()", {0}}, + {"15.3", "SELECT TOTAL_CHANGES()", {19}}, + {"16.0", "START TRANSACTION", {}}, + {"16.1", "INSERT INTO T1 VALUES(21)", {}}, + }) + +test:do_catchsql_test( + "gh2181-changes-statistics-16.2", + -- Rollback during autocommit. + "INSERT OR ROLLBACK INTO T1 VALUES(21)", + {1, "Duplicate key exists in unique index 'pk_unnamed_T1_1' in space 'T1'"}) + +test:do_select_tests( + "gh2181-changes-statistics", + { + {"16.3", "SELECT CHANGES()", {0}}, + {"16.4", "SELECT TOTAL_CHANGES()", {19}}, + }) + +test:do_catchsql_test( + "gh2181-changes-statistics-17.1", + "INSERT OR FAIL INTO T1 VALUES(31), (31)", + {1, "Duplicate key exists in unique index 'pk_unnamed_T1_1' in space 'T1'"}) + +test:do_select_tests( + "gh2181-changes-statistics", + { + {"17.2", "SELECT CHANGES()", {1}}, + {"17.3", "SELECT TOTAL_CHANGES()", {20}}, + {"18.1", "INSERT OR IGNORE INTO T1 VALUES(32), (32)", {}}, + {"18.2", "SELECT CHANGES()", {1}}, + {"18.3", "SELECT TOTAL_CHANGES()", {21}}, + }) + +test:finish_test()
prev parent reply other threads:[~2018-10-18 14:41 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-09-27 12:39 [tarantool-patches] [PATCH 0/2] Add test on changes && fix typo in MakeRecord AKhatskevich 2018-09-27 12:39 ` [tarantool-patches] [PATCH 1/2] sql: fix typo in MakeRecord memory hint AKhatskevich 2018-10-01 0:12 ` [tarantool-patches] " n.pettik 2018-09-27 12:39 ` [tarantool-patches] [PATCH 2/2] sql: add test on changes/total_changes AKhatskevich 2018-09-30 23:54 ` [tarantool-patches] " n.pettik [not found] ` <c5639a4e-0b51-cbaa-112c-4f47d86bbe07@tarantool.org> 2018-10-01 15:20 ` n.pettik 2018-10-01 16:13 ` Alex Khatskevich [not found] ` <881CB143-0DC2-468A-90CA-0459E9EE7B15@gmail.com> 2018-10-04 10:49 ` n.pettik [not found] ` <75020feb-3d2a-2707-bfdb-fd3fd2229afa@tarantool.org> 2018-10-09 12:11 ` n.pettik 2018-10-10 14:39 ` Alex Khatskevich 2018-10-18 14:41 ` n.pettik [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=828A30A0-135C-42C7-A810-230FC227C7EA@tarantool.org \ --to=korablev@tarantool.org \ --cc=avkhatskevich@tarantool.org \ --cc=kyukhin@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox