From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 8B4932D2C8 for ; Thu, 18 Oct 2018 10:41:08 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id KTsSvfPyrX72 for ; Thu, 18 Oct 2018 10:41:08 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 9945F2D222 for ; Thu, 18 Oct 2018 10:41:07 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes From: "n.pettik" In-Reply-To: <72b1a158-ef2d-50f6-2ad5-5ffcf598b4c1@tarantool.org> Date: Thu, 18 Oct 2018 17:41:03 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <828A30A0-135C-42C7-A810-230FC227C7EA@tarantool.org> References: <7BEF7461-8C36-472F-95D7-6E46CD99E956@tarantool.org> <9C1DDEBD-C37D-4B61-96AC-832BC4DDD12A@tarantool.org> <881CB143-0DC2-468A-90CA-0459E9EE7B15@gmail.com> <2DF5F7A1-C504-4B82-B394-0F16DF025D13@tarantool.org> <75020feb-3d2a-2707-bfdb-fd3fd2229afa@tarantool.org> <02140D0D-8DA6-4B74-B6A0-38572F63C745@tarantool.org> <72b1a158-ef2d-50f6-2ad5-5ffcf598b4c1@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Alex Khatskevich , Kirill Yukhin Unfortunately, I=E2=80=99ve 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=E2=80=9D) tarantool> box.sql.execute('select total_changes()') --- - - [5] =E2=80=A6 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.=20 Overall, I assume we=E2=80=99ve spent enough time for such insignificant = feature. My branch is = https://github.com/tarantool/tarantool/tree/np/gh-2181-rework-change-count= er Reworked patch is below, I am done with it. Kirill, now it is up to you: sql: increase total_changes only on commit =20 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). =20 Closes #2181 =20 @TarantoolBot document Title: SQL changes() and total_changes() behavior changes() and total_changes() functions work almost similarly to = SQLite. =20 `changes`: * shows number of changed by the last statement rows * updated after each statement * doesn't include changes made by triggers =20 `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 =20 Notes: * Changes which are counted: DELETE, UPDATE, INSERT. * Successful DDL requests are accounted as one change. * Changes made outside of SQL are not counted. =20 Usage example: ``` <> <> select changes(); -- return 1 <> 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; }; =20 /* @@ -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 =3D p->pFrame; p->pFrame =3D pFrame->pParent; p->nFrame--; - sqlite3VdbeSetChanges(db, p->nChange); pcx =3D sqlite3VdbeFrameRestore(pFrame); if (pOp->p2 =3D=3D ON_CONFLICT_ACTION_IGNORE) { /* Instruction pcx is the OP_Program that = invoked the sub-program @@ -2881,7 +2880,7 @@ case OP_Savepoint: { =20 if (p1=3D=3DSAVEPOINT_BEGIN) { /* Create a new savepoint structure. */ - pNew =3D sql_savepoint(p, zName); + pNew =3D sql_savepoint_create(zName, = psql_txn->change_count); /* Link the new savepoint into the database handle's = list. */ pNew->pNext =3D psql_txn->pSavepoint; psql_txn->pSavepoint =3D pNew; @@ -2930,6 +2929,17 @@ case OP_Savepoint: { * have to destroy them */ } + if (p1 =3D=3D SAVEPOINT_ROLLBACK) { + /* + * On savepoint rollback we should + * discard all changes made after + * it. + */ + assert(psql_txn->pSavepoint !=3D NULL); + psql_txn->change_count =3D + = psql_txn->pSavepoint->change_count; + p->nChange =3D 0; + } =20 /* 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 =3D 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 statement + * doesn't really provide any changes, it only + * accounts already made changes. + */ + assert(p->psql_txn !=3D NULL); + db->nTotalChange +=3D p->psql_txn->change_count + = p->nChange; + db->nChange =3D 0; } else { sqlite3VdbeError(p, "cannot commit - no transaction is = active"); rc =3D SQLITE_ERROR; @@ -3019,6 +3041,19 @@ case OP_TransactionRollback: { rc =3D 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 . 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 =3D 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 =3D sql_savepoint(p, NULL); + assert(p->psql_txn !=3D NULL); + uint32_t change_count =3D p->psql_txn->change_count; + p->anonymous_savepoint =3D sql_savepoint_create(NULL, + = change_count); if (p->anonymous_savepoint =3D=3D NULL) { rc =3D SQL_TARANTOOL_ERROR; goto abort_due_to_error; @@ -3806,15 +3844,13 @@ case OP_Delete: { =20 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 =3D 0; p->ignoreRaised =3D 0; break; @@ -4266,8 +4302,6 @@ case OP_IdxReplace: case OP_IdxInsert: { pIn2 =3D &aMem[pOp->p1]; assert((pIn2->flags & MEM_Blob) !=3D 0); - if (pOp->p5 & OPFLAG_NCHANGE) - p->nChange++; rc =3D ExpandBlob(pIn2); if (rc !=3D 0) goto abort_due_to_error; @@ -4293,6 +4327,9 @@ case OP_IdxInsert: { } =20 if (pOp->p5 & OPFLAG_OE_IGNORE) { + /* Compensate nChange increment. */ + if (rc !=3D SQLITE_OK) + p->nChange--; /* Ignore any kind of failes and do not raise error = message */ rc =3D SQLITE_OK; /* If we are in trigger, increment ignore raised counter = */ @@ -4305,6 +4342,8 @@ case OP_IdxInsert: { } if (rc !=3D 0) goto abort_due_to_error; + if (pOp->p5 & OPFLAG_NCHANGE) + p->nChange++; break; } =20 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 =3D NULL; + txn->change_count =3D 0; txn->fk_deferred_count =3D 0; return txn; } @@ -2270,30 +2271,27 @@ sql_txn_begin(Vdbe *p) return 0; } =20 -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 =3D zName ? strlen(zName) + 1 : 0; - size_t savepoint_sz =3D sizeof(Savepoint) + nName; - Savepoint *pNew; - - pNew =3D (Savepoint *)region_aligned_alloc(&fiber()->gc, - savepoint_sz, - alignof(Savepoint)); - if (pNew =3D=3D NULL) { - diag_set(OutOfMemory, savepoint_sz, "region", - "savepoint"); + size_t name_len =3D name !=3D NULL ? strlen(name) + 1 : 0; + size_t savepoint_sz =3D sizeof(struct Savepoint) + name_len; + struct Savepoint *savepoint =3D + (struct Savepoint *) region_alloc(&fiber()->gc, = savepoint_sz); + if (savepoint =3D=3D NULL) { + diag_set(OutOfMemory, savepoint_sz, "region", = "savepoint"); return NULL; } - pNew->tnt_savepoint =3D box_txn_savepoint(); - if (!pNew->tnt_savepoint) + savepoint->tnt_savepoint =3D box_txn_savepoint(); + if (savepoint->tnt_savepoint =3D=3D NULL) return NULL; - if (zName) { - pNew->zName =3D (char *)&pNew[1]; - memcpy(pNew->zName, zName, nName); - }; - return pNew; + savepoint->change_count =3D change_count; + if (name !=3D NULL) { + /* Name is placed right after structure itself. */ + savepoint->zName =3D (char *) &savepoint[1]; + memcpy(savepoint->zName, name, name_len); + } + return savepoint; } =20 /* @@ -2472,18 +2470,25 @@ sqlite3VdbeHalt(Vdbe * p) p->nChange =3D 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 !=3D SAVEPOINT_ROLLBACK) { - sqlite3VdbeSetChanges(db, p->nChange); + if (p->changeCntOn && !p->explain) { + db->nChange =3D p->nChange; + if (p->auto_commit) { + db->nTotalChange +=3D p->nChange; } else { - sqlite3VdbeSetChanges(db, 0); + assert(p->psql_txn !=3D NULL); + p->psql_txn->change_count +=3D = p->nChange; } - p->nChange =3D 0; } + p->nChange =3D 0; } =20 closeCursorsAndFree(p); @@ -3439,17 +3444,6 @@ sqlite3MemCompare(const Mem * pMem1, const Mem * = pMem2, const struct coll * pCol return sqlite3BlobCompare(pMem1, pMem2); } =20 -/* - * 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 =3D nChange; - db->nTotalChange +=3D 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 =3D 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 =3D 1", {}}, + {"7.1", "SELECT CHANGES()", {1}}, + {"7.2", "SELECT TOTAL_CHANGES()", {10}}, + {"8.0", "UPDATE T1 SET A =3D 1 where A =3D 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 =3D 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()=