* [tarantool-patches] [PATCH 0/2] Add test on changes && fix typo in MakeRecord @ 2018-09-27 12:39 AKhatskevich 2018-09-27 12:39 ` [tarantool-patches] [PATCH 1/2] sql: fix typo in MakeRecord memory hint AKhatskevich 2018-09-27 12:39 ` [tarantool-patches] [PATCH 2/2] sql: add test on changes/total_changes AKhatskevich 0 siblings, 2 replies; 11+ messages in thread From: AKhatskevich @ 2018-09-27 12:39 UTC (permalink / raw) To: kyukhin, tarantool-patches Issue: https://github.com/tarantool/tarantool/issues/2181 Branch: https://github.com/tarantool/tarantool/tree/kh/gh-2181-add_changes_test AKhatskevich (2): sql: fix typo in MakeRecord memory hint sql: add test on changes/total_changes src/box/sql/update.c | 2 +- test/sql-tap/gh2181-changes-statistics.test.lua | 74 +++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) create mode 100755 test/sql-tap/gh2181-changes-statistics.test.lua -- 2.14.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH 1/2] sql: fix typo in MakeRecord memory hint 2018-09-27 12:39 [tarantool-patches] [PATCH 0/2] Add test on changes && fix typo in MakeRecord AKhatskevich @ 2018-09-27 12:39 ` 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 1 sibling, 1 reply; 11+ messages in thread From: AKhatskevich @ 2018-09-27 12:39 UTC (permalink / raw) To: kyukhin, tarantool-patches Part of: #2181 Related to: #3021 --- Issue: https://github.com/tarantool/tarantool/issues/3021 src/box/sql/update.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/box/sql/update.c b/src/box/sql/update.c index 730872fc6..1069f62cb 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -276,9 +276,9 @@ sqlite3Update(Parse * pParse, /* The parser context */ pPk->def); sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, pk_part_count, regKey, zAff, pk_part_count); - sqlite3VdbeAddOp2(v, OP_IdxInsert, iEph, regKey); /* Set flag to save memory allocating one by malloc. */ sqlite3VdbeChangeP5(v, 1); + sqlite3VdbeAddOp2(v, OP_IdxInsert, iEph, regKey); } /* End the database scan loop. */ -- 2.14.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 1/2] sql: fix typo in MakeRecord memory hint 2018-09-27 12:39 ` [tarantool-patches] [PATCH 1/2] sql: fix typo in MakeRecord memory hint AKhatskevich @ 2018-10-01 0:12 ` n.pettik 0 siblings, 0 replies; 11+ messages in thread From: n.pettik @ 2018-10-01 0:12 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Yukhin, Alex Khatskevich LGTM. > On 27 Sep 2018, at 15:39, AKhatskevich <avkhatskevich@tarantool.org> wrote: > > Part of: #2181 > Related to: #3021 > --- > Issue: https://github.com/tarantool/tarantool/issues/3021 > > src/box/sql/update.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index 730872fc6..1069f62cb 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -276,9 +276,9 @@ sqlite3Update(Parse * pParse, /* The parser context */ > pPk->def); > sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, pk_part_count, > regKey, zAff, pk_part_count); > - sqlite3VdbeAddOp2(v, OP_IdxInsert, iEph, regKey); > /* Set flag to save memory allocating one by malloc. */ > sqlite3VdbeChangeP5(v, 1); > + sqlite3VdbeAddOp2(v, OP_IdxInsert, iEph, regKey); > } > /* End the database scan loop. > */ > -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH 2/2] sql: add test on changes/total_changes 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-09-27 12:39 ` AKhatskevich 2018-09-30 23:54 ` [tarantool-patches] " n.pettik 1 sibling, 1 reply; 11+ messages in thread From: AKhatskevich @ 2018-09-27 12:39 UTC (permalink / raw) To: kyukhin, tarantool-patches Closes #2181 --- test/sql-tap/gh2181-changes-statistics.test.lua | 74 +++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100755 test/sql-tap/gh2181-changes-statistics.test.lua 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..26eb95f28 --- /dev/null +++ b/test/sql-tap/gh2181-changes-statistics.test.lua @@ -0,0 +1,74 @@ +#!/usr/bin/env tarantool +test = require("sqltester") +test:plan(56) + +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. + {"6.4", "SELECT CHANGES()", {1}}, + {"6.5", "SELECT TOTAL_CHANGES()", {9}}, + {"6.6", "DROP TRIGGER TRIG1"}, + {"6.7", "SELECT CHANGES()", {1}}, + {"6.8", "SELECT TOTAL_CHANGES()", {10}}, + {"7.0", "DELETE FROM T1 WHERE A = 1", {}}, + {"7.1", "SELECT CHANGES()", {1}}, + {"7.2", "SELECT TOTAL_CHANGES()", {11}}, + {"8.0", "UPDATE T1 SET A = 1 where A = 2", {}}, + {"8.1", "SELECT CHANGES()", {1}}, + {"8.2", "SELECT TOTAL_CHANGES()", {12}}, + {"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()", {15}}, + {"11.0", "DELETE FROM T1", {}}, + {"11.1", "SELECT CHANGES()", {0}}, + {"11.2", "SELECT TOTAL_CHANGES()", {15}}, + {"12.0", "DELETE FROM T2 WHERE A < 100", {}}, + {"12.1", "SELECT CHANGES()", {3}}, + {"12.2", "SELECT TOTAL_CHANGES()", {18}}, + -- Transactions/savepoints. + {"13.0", "START TRANSACTION", {}}, + {"13.1", "INSERT INTO T1 VALUES(11)", {}}, + {"13.2", "SELECT CHANGES()", {1}}, + {"13.3", "SELECT TOTAL_CHANGES()", {19}}, + {"13.4", "SAVEPOINT S1", {}}, + {"13.5", "INSERT INTO T1 VALUES(12)", {}}, + {"13.6", "SELECT CHANGES()", {1}}, + {"13.7", "SELECT TOTAL_CHANGES()", {20}}, + {"13.8", "ROLLBACK TO SAVEPOINT S1", {}}, + {"13.9", "SELECT CHANGES()", {1}}, + {"13.10", "SELECT TOTAL_CHANGES()", {20}}, + {"13.11", "COMMIT", {}}, + {"13.12", "SELECT CHANGES()", {1}}, + {"13.13", "SELECT TOTAL_CHANGES()", {20}}, + + }) + +test:finish_test() -- 2.14.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes 2018-09-27 12:39 ` [tarantool-patches] [PATCH 2/2] sql: add test on changes/total_changes AKhatskevich @ 2018-09-30 23:54 ` n.pettik [not found] ` <c5639a4e-0b51-cbaa-112c-4f47d86bbe07@tarantool.org> 0 siblings, 1 reply; 11+ messages in thread From: n.pettik @ 2018-09-30 23:54 UTC (permalink / raw) To: tarantool-patches; +Cc: Alex Khatskevich > On 27 Sep 2018, at 15:39, AKhatskevich <avkhatskevich@tarantool.org> wrote: > > Closes #2181 > --- > test/sql-tap/gh2181-changes-statistics.test.lua | 74 +++++++++++++++++++++++++ > 1 file changed, 74 insertions(+) > create mode 100755 test/sql-tap/gh2181-changes-statistics.test.lua > > 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..26eb95f28 > --- /dev/null > +++ b/test/sql-tap/gh2181-changes-statistics.test.lua > @@ -0,0 +1,74 @@ > +#!/usr/bin/env tarantool > +test = require("sqltester") > +test:plan(56) > + > +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. > + {"6.4", "SELECT CHANGES()", {1}}, > + {"6.5", "SELECT TOTAL_CHANGES()", {9}}, > + {"6.6", "DROP TRIGGER TRIG1"}, > + {"6.7", "SELECT CHANGES()", {1}}, > + {"6.8", "SELECT TOTAL_CHANGES()", {10}}, > + {"7.0", "DELETE FROM T1 WHERE A = 1", {}}, > + {"7.1", "SELECT CHANGES()", {1}}, > + {"7.2", "SELECT TOTAL_CHANGES()", {11}}, > + {"8.0", "UPDATE T1 SET A = 1 where A = 2", {}}, > + {"8.1", "SELECT CHANGES()", {1}}, > + {"8.2", "SELECT TOTAL_CHANGES()", {12}}, > + {"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()", {15}}, > + {"11.0", "DELETE FROM T1", {}}, > + {"11.1", "SELECT CHANGES()", {0}}, > + {"11.2", "SELECT TOTAL_CHANGES()", {15}}, > + {"12.0", "DELETE FROM T2 WHERE A < 100", {}}, > + {"12.1", "SELECT CHANGES()", {3}}, > + {"12.2", "SELECT TOTAL_CHANGES()", {18}}, > + -- Transactions/savepoints. > + {"13.0", "START TRANSACTION", {}}, > + {"13.1", "INSERT INTO T1 VALUES(11)", {}}, > + {"13.2", "SELECT CHANGES()", {1}}, > + {"13.3", "SELECT TOTAL_CHANGES()", {19}}, > + {"13.4", "SAVEPOINT S1", {}}, > + {"13.5", "INSERT INTO T1 VALUES(12)", {}}, > + {"13.6", "SELECT CHANGES()", {1}}, > + {"13.7", "SELECT TOTAL_CHANGES()", {20}}, > + {"13.8", "ROLLBACK TO SAVEPOINT S1", {}}, > + {"13.9", "SELECT CHANGES()", {1}}, > + {"13.10", "SELECT TOTAL_CHANGES()", {20}}, But why here TOTAL_CHANGES == 20? At S1 savepoint we have TOTAL_CHANGES == 19, so after rollback I guess it shouldn’t change… Did I miss something? ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <c5639a4e-0b51-cbaa-112c-4f47d86bbe07@tarantool.org>]
* [tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes [not found] ` <c5639a4e-0b51-cbaa-112c-4f47d86bbe07@tarantool.org> @ 2018-10-01 15:20 ` n.pettik 2018-10-01 16:13 ` Alex Khatskevich 0 siblings, 1 reply; 11+ messages in thread From: n.pettik @ 2018-10-01 15:20 UTC (permalink / raw) To: tarantool-patches; +Cc: Alex Khatskevich > On 1 Oct 2018, at 12:09, Alex Khatskevich <avkhatskevich@tarantool.org> wrote: > >>> >>> + {"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. >>> + {"6.4", "SELECT CHANGES()", {1}}, >>> + {"6.5", "SELECT TOTAL_CHANGES()", {9}}, >>> + {"6.6", "DROP TRIGGER TRIG1"}, >>> + {"6.7", "SELECT CHANGES()", {1}}, >>> + {"6.8", "SELECT TOTAL_CHANGES()", {10}}, >>> + {"7.0", "DELETE FROM T1 WHERE A = 1", {}}, >>> + {"7.1", "SELECT CHANGES()", {1}}, >>> + {"7.2", "SELECT TOTAL_CHANGES()", {11}}, >>> + {"8.0", "UPDATE T1 SET A = 1 where A = 2", {}}, >>> + {"8.1", "SELECT CHANGES()", {1}}, >>> + {"8.2", "SELECT TOTAL_CHANGES()", {12}}, >>> + {"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()", {15}}, >>> + {"11.0", "DELETE FROM T1", {}}, >>> + {"11.1", "SELECT CHANGES()", {0}}, >>> + {"11.2", "SELECT TOTAL_CHANGES()", {15}}, >>> + {"12.0", "DELETE FROM T2 WHERE A < 100", {}}, >>> + {"12.1", "SELECT CHANGES()", {3}}, >>> + {"12.2", "SELECT TOTAL_CHANGES()", {18}}, >>> + -- Transactions/savepoints. >>> + {"13.0", "START TRANSACTION", {}}, >>> + {"13.1", "INSERT INTO T1 VALUES(11)", {}}, >>> + {"13.2", "SELECT CHANGES()", {1}}, >>> + {"13.3", "SELECT TOTAL_CHANGES()", {19}}, >>> + {"13.4", "SAVEPOINT S1", {}}, >>> + {"13.5", "INSERT INTO T1 VALUES(12)", {}}, >>> + {"13.6", "SELECT CHANGES()", {1}}, >>> + {"13.7", "SELECT TOTAL_CHANGES()", {20}}, >>> + {"13.8", "ROLLBACK TO SAVEPOINT S1", {}}, >>> + {"13.9", "SELECT CHANGES()", {1}}, >>> + {"13.10", "SELECT TOTAL_CHANGES()", {20}}, >> But why here TOTAL_CHANGES == 20? At S1 savepoint we have >> TOTAL_CHANGES == 19, so after rollback I guess it shouldn’t change… >> Did I miss something? > I have expected the same thing. > However: > 1. sqlite works in the same way. It doesn’t mean anything. SQLite features a wide range of really strange things. > 2. implementing this just need adding (nChanges) to savepoint Seems so, but it shouldn’t be complicated. > 3. I suppose that we should not implement this. Why? I guess it is matter of discussion. > The only difference with sqlite is ddl operations are counted in tarantool. > The reason is obvious (count changes flag is raised where it should not be). > I propose just to create issue on this by now (i will do it). Anyway, I propose to decide what to do with TOTAL_CHANGES within this patch, since original issue sounds like: "Fix if easy, remove otherwise.” Adding initially wrong tests is likely to be huge mistake. PS don’t forget to add mailing list to receivers. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes 2018-10-01 15:20 ` n.pettik @ 2018-10-01 16:13 ` Alex Khatskevich [not found] ` <881CB143-0DC2-468A-90CA-0459E9EE7B15@gmail.com> 0 siblings, 1 reply; 11+ messages in thread From: Alex Khatskevich @ 2018-10-01 16:13 UTC (permalink / raw) To: tarantool-patches >> I have expected the same thing. >> However: >> 1. sqlite works in the same way. > It doesn’t mean anything. SQLite features a wide range of really strange things. > >> 2. implementing this just need adding (nChanges) to savepoint > Seems so, but it shouldn’t be complicated. I suppose it is not complicated. >> 3. I suppose that we should not implement this. > Why? I guess it is matter of discussion. I think that it is not important, because it is relatively useless counter. 1. No one need to count exact number of changes. 2. Changing the api require solid understanding of what total_changes is and why we need to change this. It strange for me that it do not do rollback on rollback, however, it meant to work this way and it also makes sense. > Anyway, I propose to decide what to do with TOTAL_CHANGES within this patch, > since original issue sounds like: > > "Fix if easy, remove otherwise.” > > Adding initially wrong tests is likely to be huge mistake. This thing works. Tests proof that. I believe that those tests are the artefact of this issue. If you want change something, you need to file an issue and explain, why this change is important. ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <881CB143-0DC2-468A-90CA-0459E9EE7B15@gmail.com>]
* [tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes [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> 0 siblings, 1 reply; 11+ messages in thread From: n.pettik @ 2018-10-04 10:49 UTC (permalink / raw) To: tarantool-patches; +Cc: Alex Khatskevich Accidentally, I sent last mail from wrong address, so it didn’t arrive to mailing list. Original letter below: > On 2 Oct 2018, at 16:40, Никита Петтик <kitnerh@gmail.com> wrote: > > >>>> I have expected the same thing. >>>> However: >>>> 1. sqlite works in the same way. >>> It doesn’t mean anything. SQLite features a wide range of really strange things. >>> >>>> 2. implementing this just need adding (nChanges) to savepoint >>> Seems so, but it shouldn’t be complicated. >> I suppose it is not complicated. >>>> 3. I suppose that we should not implement this. >>> Why? I guess it is matter of discussion. >> I think that it is not important, because it is relatively useless counter. >> 1. No one need to count exact number of changes. > > Without proofs such statements worth nothing. > >> 2. Changing the api require solid understanding of what total_changes is >> and why we need to change this. >> It strange for me that it do not do rollback on rollback, however, it meant to work this >> way and it also makes sense. > > It only means that SQLite developers decided to do so, nothing else. > >>> Anyway, I propose to decide what to do with TOTAL_CHANGES within this patch, >>> since original issue sounds like: >>> >>> "Fix if easy, remove otherwise.” >>> >>> Adding initially wrong tests is likely to be huge mistake. >> This thing works. Tests proof that. > > ‘Works' and ‘works and gives wrong result' are sort of different things. > In this regard, rollback doesn’t work, ergo this counter works well > only in half cases. > >> I believe that those tests are the artefact of this issue. >> If you want change something, you need to file an issue and explain, >> why this change is important. > > I guess there is no matter of discussion: it is very likely to be obvious > that rollback should return old total_change counter (i.e. value before rollback). > If you strictly disagree, you are welcome to discuss it with other members. ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <75020feb-3d2a-2707-bfdb-fd3fd2229afa@tarantool.org>]
* [tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes [not found] ` <75020feb-3d2a-2707-bfdb-fd3fd2229afa@tarantool.org> @ 2018-10-09 12:11 ` n.pettik 2018-10-10 14:39 ` Alex Khatskevich 0 siblings, 1 reply; 11+ messages in thread From: n.pettik @ 2018-10-09 12:11 UTC (permalink / raw) To: tarantool-patches; +Cc: Alex Khatskevich > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes 2018-10-09 12:11 ` n.pettik @ 2018-10-10 14:39 ` Alex Khatskevich 2018-10-18 14:41 ` n.pettik 0 siblings, 1 reply; 11+ messages in thread From: Alex Khatskevich @ 2018-10-10 14:39 UTC (permalink / raw) To: n.pettik, tarantool-patches >> 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. Current message describe commit internals better, I suppose. >> 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* changed to repair >> 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* Checked this case. Works well (1 instead of 4) > 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. After the last patch update, zero would be returned instead of 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. I suggest do that on the first request from a user, not before. >> 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. Done, by incrementing total_changes only on a commit. >> + 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. There could be several vdbes between two savepoints. psql_txn accumulates them. >> 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 :) applied Full diff: commit 502b1b09083a300bc79f06ea1d1d64b6c686fe6f Author: AKhatskevich <avkhatskevich@tarantool.org> Date: Thu Sep 27 15:28:52 2018 +0300 sql: total_changes add test && change behavior This commit repair changes/total_changes. Key points: * Add tests. * total_changes is incremented only on commit. * Increment changes only in case operation succeeds. Closes #2181 @TarantoolBot document Title: sql changes/total_changes behavior changes/total_changes works 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 * updates on commit (or auto-commit) * include changes made by triggers Notes: * Changes which are counted: delete, update, insert. * Those statistics are not persisted. * Changes made outside of sql are not counted. Usage example: ``` <<db started>> <<statement: delete 1 row>> <<statement: insert 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 53188e74d..af3ca3c57 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1763,6 +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) */ + /** Changes to db made since last START TRANSACTION. */ + int change_count; }; /* @@ -4305,7 +4307,17 @@ 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); + +/** + * This routine sets the value to be returned by subsequent + * calls to changes() and total_changes(). + * + * @param v Active Vdbe. + * @param transaction_end Indicator for total_changes update. + */ +void +vdbe_changes_update(struct Vdbe *v, bool transaction_end); + 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..56c23e936 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); + vdbe_changes_update(p, false); 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_create(p, 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 +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 != NULL); + psql_txn->change_count = + psql_txn->pSavepoint->change_count; + } /* 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,7 @@ case OP_TransactionCommit: { rc = SQL_TARANTOOL_ERROR; goto abort_due_to_error; } + vdbe_changes_update(p, true); } else { sqlite3VdbeError(p, "cannot commit - no transaction is active"); rc = SQLITE_ERROR; @@ -3019,6 +3030,7 @@ case OP_TransactionRollback: { rc = SQL_TARANTOOL_ERROR; goto abort_due_to_error; } + vdbe_changes_update(p, false); } else { sqlite3VdbeError(p, "cannot rollback - no " "transaction is active"); @@ -3045,7 +3057,10 @@ case OP_TTransaction: { goto abort_due_to_error; } } else { - p->anonymous_savepoint = sql_savepoint(p, NULL); + assert(p->psql_txn); + int 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; @@ -3836,7 +3851,7 @@ case OP_Delete: { * This is used by trigger programs. */ case OP_ResetCount: { - sqlite3VdbeSetChanges(db, p->nChange); + vdbe_changes_update(p, true); p->nChange = 0; p->ignoreRaised = 0; break; @@ -4277,7 +4292,6 @@ case OP_IdxInsert: { /* in2 */ assert(isSorter(pC)==(pOp->opcode==OP_SorterInsert)); pIn2 = &aMem[pOp->p2]; assert(pIn2->flags & MEM_Blob); - if (pOp->p5 & OPFLAG_NCHANGE) p->nChange++; assert(pC->eCurType==CURTYPE_TARANTOOL || pOp->opcode==OP_SorterInsert); rc = ExpandBlob(pIn2); if (rc) goto abort_due_to_error; @@ -4307,6 +4321,9 @@ case OP_IdxInsert: { /* in2 */ } 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 */ @@ -4319,6 +4336,8 @@ case OP_IdxInsert: { /* in2 */ p->errorAction = ON_CONFLICT_ACTION_ROLLBACK; } if (rc) goto abort_due_to_error; + if ((pOp->p5 & OPFLAG_NCHANGE) != 0) + p->nChange++; break; } diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index ce97f4984..cbdb8d584 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(struct Vdbe *p, const char *name, int 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 54edf1b03..284e1deec 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,28 @@ sql_txn_begin(Vdbe *p) return 0; } -Savepoint * -sql_savepoint(Vdbe *p, const char *zName) +struct Savepoint * +sql_savepoint_create(struct Vdbe *p, const char *name, int 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_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; } /* @@ -2446,7 +2445,8 @@ sqlite3VdbeHalt(Vdbe * p) closeCursorsAndFree(p); sqlite3RollbackAll(p); sqlite3CloseSavepoints(p); - p->nChange = 0; + assert(p->psql_txn); + p->nChange = -p->psql_txn->change_count; } } @@ -2477,12 +2477,11 @@ sqlite3VdbeHalt(Vdbe * p) * has been rolled back, update the database connection change-counter. */ if (p->changeCntOn) { - if (eStatementOp != SAVEPOINT_ROLLBACK) { - sqlite3VdbeSetChanges(db, p->nChange); - } else { - sqlite3VdbeSetChanges(db, 0); + if (eStatementOp == SAVEPOINT_ROLLBACK) { + p->nChange = 0; + p->psql_txn->change_count = 0; } - p->nChange = 0; + vdbe_changes_update(p, p->auto_commit); } } @@ -3439,15 +3438,23 @@ 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) +vdbe_changes_update(struct Vdbe *p, bool transaction_end) { - db->nChange = nChange; - db->nTotalChange += nChange; + struct sqlite3 *db = p->db; + struct sql_txn *psql_txn = p->psql_txn; + db->nChange = p->nChange; + if (transaction_end) { + db->nTotalChange += p->nChange; + if (psql_txn != NULL) { + db->nTotalChange += psql_txn->change_count; + psql_txn->change_count = 0; + } + } else { + if (psql_txn != NULL) + psql_txn->change_count += p->nChange; + } + p->nChange = 0; } /* diff --git a/src/box/txn.h b/src/box/txn.h index e74c14d40..4f4c86347 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(). + */ + int 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..14f0999ae --- /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. + {"6.4", "SELECT CHANGES()", {1}}, + {"6.5", "SELECT TOTAL_CHANGES()", {9}}, + {"6.6", "DROP TRIGGER TRIG1"}, + {"6.7", "SELECT CHANGES()", {1}}, + {"6.8", "SELECT TOTAL_CHANGES()", {10}}, + {"7.0", "DELETE FROM T1 WHERE A = 1", {}}, + {"7.1", "SELECT CHANGES()", {1}}, + {"7.2", "SELECT TOTAL_CHANGES()", {11}}, + {"8.0", "UPDATE T1 SET A = 1 where A = 2", {}}, + {"8.1", "SELECT CHANGES()", {1}}, + {"8.2", "SELECT TOTAL_CHANGES()", {12}}, + {"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()", {15}}, + {"11.0", "DELETE FROM T1", {}}, + {"11.1", "SELECT CHANGES()", {0}}, + {"11.2", "SELECT TOTAL_CHANGES()", {15}}, + {"12.0", "DELETE FROM T2 WHERE A < 100", {}}, + {"12.1", "SELECT CHANGES()", {3}}, + {"12.2", "SELECT TOTAL_CHANGES()", {18}}, + -- Transactions/savepoints. + {"13.0", "START TRANSACTION", {}}, + {"13.1", "INSERT INTO T1 VALUES(11)", {}}, + {"13.2", "SELECT CHANGES()", {1}}, + {"13.3", "SELECT TOTAL_CHANGES()", {18}}, + {"13.4", "SAVEPOINT S1", {}}, + {"13.5", "INSERT INTO T1 VALUES(12)", {}}, + {"13.6", "SELECT CHANGES()", {1}}, + {"13.7", "SELECT TOTAL_CHANGES()", {18}}, + {"13.8", "ROLLBACK TO SAVEPOINT S1", {}}, + {"13.9", "SELECT CHANGES()", {1}}, + {"13.10", "SELECT TOTAL_CHANGES()", {18}}, + {"13.11", "COMMIT", {}}, + {"13.12", "SELECT CHANGES()", {0}}, + {"13.13", "SELECT TOTAL_CHANGES()", {19}}, + {"14.0", "START TRANSACTION", {}}, + {"14.1", "INSERT INTO T1 VALUES(15)", {}}, + {"14.2", "SELECT CHANGES()", {1}}, + {"14.3", "SELECT TOTAL_CHANGES()", {19}}, + {"14.4", "ROLLBACK", {}}, + {"14.5", "SELECT CHANGES()", {0}}, + {"14.6", "SELECT TOTAL_CHANGES()", {19}}, + {"15.0.1", "INSERT INTO T1 VALUES(1)", {}}, + {"15.0.2", "SELECT TOTAL_CHANGES()", {20}}, + }) + +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()", {20}}, + {"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()", {-1}}, + {"16.4", "SELECT TOTAL_CHANGES()", {20}}, + }) + +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()", {21}}, + {"18.1", "INSERT OR IGNORE INTO T1 VALUES(32), (32)", {}}, + {"18.2", "SELECT CHANGES()", {1}}, + {"18.3", "SELECT TOTAL_CHANGES()", {22}}, + }) + +test:finish_test() ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: add test on changes/total_changes 2018-10-10 14:39 ` Alex Khatskevich @ 2018-10-18 14:41 ` n.pettik 0 siblings, 0 replies; 11+ messages in thread From: n.pettik @ 2018-10-18 14:41 UTC (permalink / raw) To: tarantool-patches; +Cc: Alex Khatskevich, Kirill Yukhin 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() ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-10-18 14:41 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox