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 3148C2B992 for ; Tue, 9 Oct 2018 08:12:01 -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 1tLFYEJ95Bcb for ; Tue, 9 Oct 2018 08:12:01 -0400 (EDT) Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 664712B90B for ; Tue, 9 Oct 2018 08:12:00 -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: <75020feb-3d2a-2707-bfdb-fd3fd2229afa@tarantool.org> Date: Tue, 9 Oct 2018 15:11:54 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <02140D0D-8DA6-4B74-B6A0-38572F63C745@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> 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 > commit 4b72b445851fb8c14313f2ed15b87b1c6b792b13 > Author: AKhatskevich > Date: Thu Sep 27 15:28:52 2018 +0300 >=20 > sql: total_changes add test && change behavior Nit: total_changes and what? Don=E2=80=99t 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 =E2=80=98relive' is "Live through (an experience or feeling, especially an unpleasant one) again in one's imagination or memory.=E2=80=9D So I guess = =E2=80=9Cresurrect=E2=80=9D or =E2=80=9Crepair" 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. >=20 > 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=E2=80=99t notice it if you didn=E2=80=99t 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=E2=80=99t 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=E2=80=99t be taken into consideration. Personally, I don=E2=80=99= t think that it is sort of =E2=80=98incorrect=E2=80=99 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 =3D p->pFrame; > p->pFrame =3D pFrame->pParent; > p->nFrame--; > - sqlite3VdbeSetChanges(db, p->nChange); > + sqlite3VdbeSetChanges(p); > 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 +2881,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(p, zName, psql_txn->n_change); > /* Link the new savepoint into the database handle's list. */ > pNew->pNext =3D psql_txn->pSavepoint; > psql_txn->pSavepoint =3D pNew; > @@ -2930,6 +2930,16 @@ case OP_Savepoint: { > * have to destroy them > */ > } > + if (p1 =3D=3D SAVEPOINT_ROLLBACK) { > + /* > + * Set nChanges right there, because savepoint > + * statements do not call halt. > + */ > + assert(psql_txn->pSavepoint); > + p->nChange =3D psql_txn->pSavepoint->n_change - > + psql_txn->n_change; > + sqlite3VdbeSetChanges(p); > + } >=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 > @@ -3019,6 +3029,8 @@ case OP_TransactionRollback: { > rc =3D SQL_TARANTOOL_ERROR; > goto abort_due_to_error; > } > + p->nChange =3D -p->psql_txn->n_change; I don=E2=80=99t like that changes counter can take negative values - it = may be quite misleading. Lets avoid this behaviour, i.e. make sure that changes = always >=3D 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 =3D sql_savepoint(p, NULL); > + assert(p->psql_txn); Nit: we use explicit !=3D 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=E2=80=99ve 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 =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)", {}}, > + -- 1 instead of 2; sqlite is the same. AFAIR we came to agreement to avoid mentions about SQLite in our = sources=E2=80=A6 Moreover, comments says =E2=80=981 instead of 2=E2=80=99 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 =3D changes_1 + changes_2 + =E2=80=A6 At least you should describe this behaviour in docs.=20 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; }; =20 /* @@ -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 =3D p->pFrame; p->pFrame =3D pFrame->pParent; p->nFrame--; - sqlite3VdbeSetChanges(p); + vdbe_changes_update(p); 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 +2881,7 @@ case OP_Savepoint: { =20 if (p1=3D=3DSAVEPOINT_BEGIN) { /* Create a new savepoint structure. */ - pNew =3D sql_savepoint(p, zName, psql_txn->n_change); + pNew =3D sql_savepoint_create(p, zName, = psql_txn->n_change); /* Link the new savepoint into the database handle's = list. */ pNew->pNext =3D psql_txn->pSavepoint; psql_txn->pSavepoint =3D pNew; @@ -2932,13 +2932,14 @@ case OP_Savepoint: { } if (p1 =3D=3D 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 =3D = psql_txn->pSavepoint->n_change - - psql_txn->n_change; - sqlite3VdbeSetChanges(p); + assert(psql_txn->pSavepoint !=3D NULL); + p->nChange =3D = psql_txn->pSavepoint->change_count - + psql_txn->change_count; + vdbe_changes_update(p); } =20 /* If it is a RELEASE, then destroy the = savepoint being operated on @@ -3029,8 +3030,8 @@ case OP_TransactionRollback: { rc =3D SQL_TARANTOOL_ERROR; goto abort_due_to_error; } - p->nChange =3D -p->psql_txn->n_change; - sqlite3VdbeSetChanges(p); + p->nChange =3D -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 =3D p->psql_txn->n_change; - p->anonymous_savepoint =3D sql_savepoint(p, NULL, = new_changes); + uint32_t new_changes =3D p->psql_txn->change_count; + p->anonymous_savepoint =3D sql_savepoint_create(p, NULL, + = new_changes); if (p->anonymous_savepoint =3D=3D NULL) { rc =3D 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 =3D 0; p->ignoreRaised =3D 0; break; @@ -4320,7 +4322,7 @@ case OP_IdxInsert: { /* in2 */ } =20 if (pOp->p5 & OPFLAG_OE_IGNORE) { - /* Compensate nChange increment */ + /* Compensate nChange increment. */ if (rc !=3D 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 =3D ON_CONFLICT_ACTION_ROLLBACK; } if (rc) goto abort_due_to_error; - if (pOp->p5 & OPFLAG_NCHANGE) p->nChange++; + if ((pOp->p5 & OPFLAG_NCHANGE) !=3D 0) + p->nChange++; break; } =20 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 =3D NULL; - txn->n_change =3D 0; + txn->change_count =3D 0; txn->fk_deferred_count =3D 0; return txn; } @@ -2271,31 +2271,28 @@ sql_txn_begin(Vdbe *p) return 0; } =20 -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 =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"); + assert(p !=3D NULL); + size_t name_len =3D name !=3D NULL ? strlen(name) + 1 : 0; + size_t savepoint_sz =3D sizeof(struct Savepoint) + name; + 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; - pNew->n_change =3D n_change; - 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 /* @@ -2483,7 +2480,7 @@ sqlite3VdbeHalt(Vdbe * p) if (eStatementOp =3D=3D SAVEPOINT_ROLLBACK) { p->nChange =3D 0; } - sqlite3VdbeSetChanges(p); + vdbe_changes_update(p); } } =20 @@ -3440,20 +3437,15 @@ 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(struct Vdbe *p) +vdbe_changes_update(struct Vdbe *p) { - sqlite3 *db =3D p->db; - int n_change =3D p->nChange; - db->nChange =3D n_change; - db->nTotalChange +=3D n_change; + struct sqlite3 *db =3D p->db; + db->nChange =3D p->nChange;; + db->nTotalChange +=3D p->nChange;; struct sql_txn *psql_txn =3D p->psql_txn; - if (psql_txn) - psql_txn->n_change +=3D n_change; + if (psql_txn !=3D NULL) + psql_txn->change_count +=3D p->nChange;; } =20 /* 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.=