[tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes
n.pettik
korablev at tarantool.org
Thu Oct 18 17:41:03 MSK 2018
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()
More information about the Tarantool-patches
mailing list