[tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes

["n.pettik" <korablev@tarantool.org>]

> commit 4b72b445851fb8c14313f2ed15b87b1c6b792b13
> Author: AKhatskevich <avkhatskevich@tarantool.org>
> Date:   Thu Sep 27 15:28:52 2018 +0300
> 
>     sql: total_changes add test && change behavior

Nit: total_changes and what? Don’t try to fit everything
in title, you can extend it with details in commit message.
I would call it simply like:

sql: restore total_changes after rollback

Change of user-observable behaviour implies that tests must
be included in patch.

>     This commit relives changes/total_changes from the Sqlite.

Nit: according to Oxford dictionary ‘relive' is
"Live through (an experience or feeling, especially an unpleasant
one) again in one's imagination or memory.” So I guess “resurrect”
or “repair" would be much more suitable here.
*sorry for being too picky*

>     Key points:
>     * Add tests.
>     * Change rollback behavior. In case an operation is not applied,
>       the total_changes counter is decreased back.
> 
>     Closes #2181

I found that some cases still seem to be invalid in their behaviour:

CREATE TABLE t1 (id INT PRIMARY KEY);
INSERT OR FAIL VALUES (1), (1), (1);
SELECT changes();
---
- - [1]
...


SELECT total_changes();
---
- - [4]
...

Total changes counted is incremented by 3, but only one insertion
was successful.
*I wouldn’t notice it if you didn’t start to fix IGNORE conflict action*

Next example:

tarantool> box.sql.execute('start transaction') box.sql.execute('insert into t1 values(3), (4),(5)') box.sql.execute('rollback')
---
...

tarantool> box.sql.execute('select changes()')
---
- - [3]
...

tarantool> box.sql.execute('select * from t1')
---
- []
...

Why does savepoint rollback set changes to negative value, but regular rollback
doesn’t even nullify it? Total changes counter is also incremented by 3.

Also, original doc concerning total_changes (https://www.sqlite.org/c3ref/total_changes.html)
says that changes provided by INSTEAD OF triggers are not counted.
Add test on this case pls as well pls.

What is more, I guess we need way to reset this counter: originally (in SQLite) this
counter is reset when connection is closed. In our implementation total_changes()
turns out to be global and never reset. We can implement it as a pragma for instance.

> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 53188e74d..e3c2041da 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -1763,6 +1763,7 @@ struct Savepoint {
>      box_txn_savepoint_t *tnt_savepoint; /* Tarantool's savepoint struct */
>      char *zName;        /* Savepoint name (nul-terminated) */
>      Savepoint *pNext;    /* Parent savepoint (if any) */
> +    int n_change;        /* Changes made since "START TRANSACTOIN" */

Still one of problems with counter is following: if changes come from non-SQL-land,
they won’t be taken into consideration. Personally, I don’t think that it is sort of
‘incorrect’ behaviour, but it definitely should be mentioned in docs. Hence, create
doc-bot request and briefly describe this feature and how it works.

I spotted a lot of codestyle violations. I put diff at the end of letter.

>  /*
> @@ -4305,7 +4306,7 @@ 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);
> +void sqlite3VdbeSetChanges(struct Vdbe *);
>  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 00ffb0c5d..30f460216 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -988,7 +988,7 @@ case OP_Halt: {
>          pFrame = p->pFrame;
>          p->pFrame = pFrame->pParent;
>          p->nFrame--;
> -        sqlite3VdbeSetChanges(db, p->nChange);
> +        sqlite3VdbeSetChanges(p);
>          pcx = sqlite3VdbeFrameRestore(pFrame);
>          if (pOp->p2 == ON_CONFLICT_ACTION_IGNORE) {
>              /* Instruction pcx is the OP_Program that invoked the sub-program
> @@ -2881,7 +2881,7 @@ case OP_Savepoint: {
> 
>      if (p1==SAVEPOINT_BEGIN) {
>          /* Create a new savepoint structure. */
> -        pNew = sql_savepoint(p, zName);
> +        pNew = sql_savepoint(p, zName, psql_txn->n_change);
>          /* Link the new savepoint into the database handle's list. */
>          pNew->pNext = psql_txn->pSavepoint;
>          psql_txn->pSavepoint = pNew;
> @@ -2930,6 +2930,16 @@ case OP_Savepoint: {
>                   * have to destroy them
>                   */
>              }
> +            if (p1 == SAVEPOINT_ROLLBACK) {
> +                /*
> +                 * Set nChanges right there, because savepoint
> +                 * statements do not call halt.
> +                 */
> +                assert(psql_txn->pSavepoint);
> +                p->nChange = psql_txn->pSavepoint->n_change -
> +                    psql_txn->n_change;
> +                sqlite3VdbeSetChanges(p);
> +            }
> 
>              /* 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
> @@ -3019,6 +3029,8 @@ case OP_TransactionRollback: {
>              rc = SQL_TARANTOOL_ERROR;
>              goto abort_due_to_error;
>          }
> +        p->nChange = -p->psql_txn->n_change;

I don’t like that changes counter can take negative values - it may be quite
misleading. Lets avoid this behaviour, i.e. make sure that changes always >= 0.

> +        sqlite3VdbeSetChanges(p);
>      } else {
>          sqlite3VdbeError(p, "cannot rollback - no "
>                      "transaction is active");
> @@ -3045,7 +3057,9 @@ case OP_TTransaction: {
>              goto abort_due_to_error;
>          }
>      } else {
> -        p->anonymous_savepoint = sql_savepoint(p, NULL);
> +        assert(p->psql_txn);

Nit: we use explicit != NULL check. I fixed almost all such places
in the diff - see at the end of letter.

> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index ce97f4984..72cd45ab4 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -442,7 +442,8 @@ int
>  sql_txn_begin(Vdbe *p);
>  Savepoint *
>  sql_savepoint(Vdbe *p,
> -          const char *zName);
> +          const char *zName,
> +          int n_change);

Looks awful. I’ve fixed that.

>  /*
> diff --git a/src/box/txn.h b/src/box/txn.h
> index e74c14d40..137cc6491 100644
> --- a/src/box/txn.h
> +++ b/src/box/txn.h
> @@ -112,6 +112,8 @@ extern double too_long_threshold;
>  struct sql_txn {
>      /** List of active SQL savepoints. */
>      struct Savepoint *pSavepoint;
> +    /** Changes made in that transaction */
> +    int n_change;

Explain pls, why do you need to store changes in both - transaction structure and
SQL savepoint? AFAIK local changes (i.e. simple changes()) are stored in VDBE (p->nChange);
total changes are stored in db; so, to rollback to previous counter value you need only to
store within save point this value.

> 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..7fae52cee
> --- /dev/null
> +++ b/test/sql-tap/gh2181-changes-statistics.test.lua
> @@ -0,0 +1,125 @@
> +#!/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)", {}},
> +        -- 1 instead of 2; sqlite is the same.

AFAIR we came to agreement to avoid mentions about SQLite in our sources…
Moreover, comments says ‘1 instead of 2’ and local change counter is really 1.
On the other hand, total changes have been incremented by 2: 7 -> 9.
I expect that total changes should be addictive in its measure:
total changes = changes_1 + changes_2 + …
At least you should describe this behaviour in docs. 

My diff is below. It consists of only cosmetic changes,
so I strongly recommend to apply it :)

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index e3c2041da..f27162ce1 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1763,7 +1763,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) */
-       int n_change;           /* Changes made since "START TRANSACTOIN" */
+       /** Changes to db made since last START TRANSACTION. */
+       uint32_t change_count;
 };
 
 /*
@@ -4306,7 +4307,14 @@ Expr *sqlite3ExprAddCollateToken(Parse * pParse, Expr *, const Token *, int);
 Expr *sqlite3ExprAddCollateString(Parse *, Expr *, const char *);
 Expr *sqlite3ExprSkipCollate(Expr *);
 int sqlite3CheckIdentifierName(Parse *, char *);
-void sqlite3VdbeSetChanges(struct Vdbe *);
+
+/**
+ * This routine sets the value to be returned by subsequent
+ * calls to sqlite3_changes() on the database handle.
+ */
+void
+vdbe_changes_update(struct Vdbe *v);
+
 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 30f460216..3d4f0b241 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -988,7 +988,7 @@ case OP_Halt: {
                pFrame = p->pFrame;
                p->pFrame = pFrame->pParent;
                p->nFrame--;
-               sqlite3VdbeSetChanges(p);
+               vdbe_changes_update(p);
                pcx = sqlite3VdbeFrameRestore(pFrame);
                if (pOp->p2 == ON_CONFLICT_ACTION_IGNORE) {
                        /* Instruction pcx is the OP_Program that invoked the sub-program
@@ -2881,7 +2881,7 @@ case OP_Savepoint: {
 
        if (p1==SAVEPOINT_BEGIN) {
                /* Create a new savepoint structure. */
-               pNew = sql_savepoint(p, zName, psql_txn->n_change);
+               pNew = sql_savepoint_create(p, zName, psql_txn->n_change);
                /* Link the new savepoint into the database handle's list. */
                pNew->pNext = psql_txn->pSavepoint;
                psql_txn->pSavepoint = pNew;
@@ -2932,13 +2932,14 @@ case OP_Savepoint: {
                        }
                        if (p1 == SAVEPOINT_ROLLBACK) {
                                /*
-                                * Set nChanges right there, because savepoint
-                                * statements do not call halt.
+                                * Set nChanges right there,
+                                * because savepoint statements
+                                * do not call halt.
                                 */
-                               assert(psql_txn->pSavepoint);
-                               p->nChange = psql_txn->pSavepoint->n_change -
-                                       psql_txn->n_change;
-                               sqlite3VdbeSetChanges(p);
+                               assert(psql_txn->pSavepoint != NULL);
+                               p->nChange = psql_txn->pSavepoint->change_count -
+                                            psql_txn->change_count;
+                               vdbe_changes_update(p);
                        }
 
                        /* If it is a RELEASE, then destroy the savepoint being operated on
@@ -3029,8 +3030,8 @@ case OP_TransactionRollback: {
                        rc = SQL_TARANTOOL_ERROR;
                        goto abort_due_to_error;
                }
-               p->nChange = -p->psql_txn->n_change;
-               sqlite3VdbeSetChanges(p);
+               p->nChange = -p->psql_txn->change_count;
+               vdbe_changes_update(p);
        } else {
                sqlite3VdbeError(p, "cannot rollback - no "
                                    "transaction is active");
@@ -3058,8 +3059,9 @@ case OP_TTransaction: {
                }
        } else {
                assert(p->psql_txn);
-               int new_changes = p->psql_txn->n_change;
-               p->anonymous_savepoint = sql_savepoint(p, NULL, new_changes);
+               uint32_t new_changes = p->psql_txn->change_count;
+               p->anonymous_savepoint = sql_savepoint_create(p, NULL,
+                                                             new_changes);
                if (p->anonymous_savepoint == NULL) {
                        rc = SQL_TARANTOOL_ERROR;
                        goto abort_due_to_error;
@@ -3850,7 +3852,7 @@ case OP_Delete: {
  * This is used by trigger programs.
  */
 case OP_ResetCount: {
-       sqlite3VdbeSetChanges(p);
+       vdbe_changes_update(p);
        p->nChange = 0;
        p->ignoreRaised = 0;
        break;
@@ -4320,7 +4322,7 @@ case OP_IdxInsert: {        /* in2 */
        }
 
        if (pOp->p5 & OPFLAG_OE_IGNORE) {
-               /* Compensate nChange increment */
+               /* Compensate nChange increment. */
                if (rc != SQLITE_OK)
                        p->nChange--;
                /* Ignore any kind of failes and do not raise error message */
@@ -4335,7 +4337,8 @@ case OP_IdxInsert: {        /* in2 */
                p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
        }
        if (rc) goto abort_due_to_error;
-       if (pOp->p5 & OPFLAG_NCHANGE) p->nChange++;
+       if ((pOp->p5 & OPFLAG_NCHANGE) != 0)
+               p->nChange++;
        break;
 }
 
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 72cd45ab4..0ac3cbe2f 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -440,10 +440,10 @@ int sqlite3VdbeExec(Vdbe *);
 int sqlite3VdbeList(Vdbe *);
 int
 sql_txn_begin(Vdbe *p);
-Savepoint *
-sql_savepoint(Vdbe *p,
-             const char *zName,
-             int n_change);
+
+struct Savepoint *
+sql_savepoint_create(struct Vdbe *p, 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 215e6597e..9f13f35f0 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -85,7 +85,7 @@ sql_alloc_txn()
                return NULL;
        }
        txn->pSavepoint = NULL;
-       txn->n_change = 0;
+       txn->change_count = 0;
        txn->fk_deferred_count = 0;
        return txn;
 }
@@ -2271,31 +2271,28 @@ sql_txn_begin(Vdbe *p)
        return 0;
 }
 
-Savepoint *
-sql_savepoint(Vdbe *p, const char *zName, int n_change)
+struct Savepoint *
+sql_savepoint_create(struct Vdbe *p, 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");
+       assert(p != NULL);
+       size_t name_len = name != NULL ? strlen(name) + 1 : 0;
+       size_t savepoint_sz = sizeof(struct Savepoint) + name;
+       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;
-       pNew->n_change = n_change;
-       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;
 }
 
 /*
@@ -2483,7 +2480,7 @@ sqlite3VdbeHalt(Vdbe * p)
                        if (eStatementOp == SAVEPOINT_ROLLBACK) {
                                p->nChange = 0;
                        }
-                       sqlite3VdbeSetChanges(p);
+                       vdbe_changes_update(p);
                }
        }
 
@@ -3440,20 +3437,15 @@ 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(struct Vdbe *p)
+vdbe_changes_update(struct Vdbe *p)
 {
-       sqlite3 *db = p->db;
-       int n_change = p->nChange;
-       db->nChange = n_change;
-       db->nTotalChange += n_change;
+       struct sqlite3 *db = p->db;
+       db->nChange = p->nChange;;
+       db->nTotalChange += p->nChange;;
        struct sql_txn *psql_txn = p->psql_txn;
-       if (psql_txn)
-               psql_txn->n_change += n_change;
+       if (psql_txn != NULL)
+               psql_txn->change_count += p->nChange;;
 }
 
 /*
diff --git a/src/box/txn.h b/src/box/txn.h
index 137cc6491..7d9fbbd51 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -112,8 +112,12 @@ extern double too_long_threshold;
 struct sql_txn {
        /** List of active SQL savepoints. */
        struct Savepoint *pSavepoint;
-       /** Changes made in that transaction */
-       int n_change;
+       /**
+        * 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.