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 AC14E23A09 for ; Fri, 4 May 2018 10:12:33 -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 rWELcBlcaSlZ for ; Fri, 4 May 2018 10:12:33 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 3BEC523926 for ; Fri, 4 May 2018 10:12:32 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions References: <503f7e68588cfa6933aab8b068509b5675da6a9c.1525368399.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 4 May 2018 17:12:30 +0300 MIME-Version: 1.0 In-Reply-To: <503f7e68588cfa6933aab8b068509b5675da6a9c.1525368399.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Nikita Pettik , tarantool-patches@freelists.org Hello. Thanks for contributing! See below 12 comments. On 03/05/2018 21:49, Nikita Pettik wrote: > This patch makes possible to start transaction in Lua and continue > operations in SQL as well, and vice versa. Previously, such transactions > result in assertion fault. To support them, it is required to hold deferred > foreign keys contrains as attributes of transaction, not particular VDBE. 1. contrains -> constraints. > Thus, deferred foreign keys counters have been completely removed from > VDBE and transfered to sql_txn struct. In its turn, if there is at least > one deferred foreign key violation, error will be raised and prevent > from commiting. Note that in this case rollback doesn't happen: > transaction becomes open untill all deferred FK violations are resolved 2. untill -> until. > OR explicit ROLLBACK statement is used. > > Also, 'PRAGMA defer_foreign_keys' has been slightly changed: now it is not > automatically turned off after trasaction's rollback or commit. It > can be turned off by explicit PRAGMA statement only. It was made owing > to the fact that execution of PRAGMA statement occurs in auto-commit > mode, so it ends with COMMIT. Hence, it turns off right after turning on > (outside the transaction). > > Closes #3237 > --- > src/box/errcode.h | 1 + > src/box/sql/fkey.c | 66 ++++++---------- > src/box/sql/main.c | 5 -- > src/box/sql/pragma.c | 3 - > src/box/sql/sqliteInt.h | 2 - > src/box/sql/status.c | 3 +- > src/box/sql/vdbe.c | 25 +++--- > src/box/sql/vdbeInt.h | 26 +------ > src/box/sql/vdbeapi.c | 3 - > src/box/sql/vdbeaux.c | 53 +++++++------ > src/box/txn.c | 18 ++++- > src/box/txn.h | 22 +++++- > test/box/misc.result | 1 + > test/sql/transitive-transactions.result | 124 ++++++++++++++++++++++++++++++ > test/sql/transitive-transactions.test.lua | 67 ++++++++++++++++ > 15 files changed, 298 insertions(+), 121 deletions(-) > create mode 100644 test/sql/transitive-transactions.result > create mode 100644 test/sql/transitive-transactions.test.lua > > diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c > index fb9a3101a..55edf6ba5 100644 > --- a/src/box/sql/fkey.c > +++ b/src/box/sql/fkey.c > @@ -748,23 +748,19 @@ fkTriggerDelete(sqlite3 * dbMem, Trigger * p) > } > } > > -/* > - * This function is called to generate code that runs when table pTab is > - * being dropped from the database. The SrcList passed as the second argument > - * to this function contains a single entry guaranteed to resolve to > - * table pTab. > - * > - * Normally, no code is required. However, if either > - * > - * (a) The table is the parent table of a FK constraint, or > - * (b) The table is the child table of a deferred FK constraint and it is > - * determined at runtime that there are outstanding deferred FK > - * constraint violations in the database, > - * > - * then the equivalent of "DELETE FROM " is executed in a single transaction > - * before dropping the table from the database. If any FK violations occur, > - * rollback transaction and halt VDBE. Triggers are disabled while running this > - * DELETE, but foreign key actions are not. > +/** > + * This function is called to generate code that runs when table > + * pTab is being dropped from the database. The SrcList passed as > + * the second argument to this function contains a single entry > + * guaranteed to resolve to table pTab. > + * > + * Normally, no code is required. However, if the table is > + * parent table of a FK constraint, then the equivalent > + * of "DELETE FROM " is executed in a single transaction > + * before dropping the table from the database. If any FK > + * violations occur, rollback transaction and halt VDBE. Triggers > + * are disabled while running this DELETE, but foreign key > + * actions are not. 3. Why did you delete this comment "The table is the child table of a deferred FK constraint" and its code? > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index 0bd0d455b..991ef9bec 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -65,16 +65,31 @@ sqlite3VdbeCreate(Parse * pParse) > db->pVdbe = p; > p->magic = VDBE_MAGIC_INIT; > p->pParse = pParse; > - p->auto_commit = !box_txn(); > p->schema_ver = box_schema_version(); > - if (!p->auto_commit) { > - p->psql_txn = in_txn()->psql_txn; > - p->nDeferredCons = p->psql_txn->nDeferredConsSave; > - p->nDeferredImmCons = p->psql_txn->nDeferredImmConsSave; > - } else{ > + struct txn *txn = in_txn(); > + p->auto_commit = txn == NULL ? true : false; 4. txn == NULL is enough. The result is boolean already. 5. sqlite3VdbeCreate is called during parsing. When prepared statements will be introduced, auto_commit and psql_txn can become garbage on the second execution. Can you please move these transaction initialization things into a function, that prepares a compiled Vdbe for further execution? > + if (txn != NULL) { > + /* > + * If transaction has been started in Lua, then > + * sql_txn is NULL. On the other hand, it is not > + * critical, since in Lua it is impossible to > + * check FK violations, at least now. > + */ > + if (txn->psql_txn == NULL) { > + txn->psql_txn = region_alloc_object(&fiber()->gc, > + struct sql_txn); > + if (txn->psql_txn == NULL) { > + sqlite3DbFree(db, p); > + diag_set(OutOfMemory, sizeof(struct sql_txn), > + "region", "struct sql_txn"); > + return NULL; 6. It is already the second place, where psql_txn is created. Lets move this into a function. > diff --git a/src/box/txn.c b/src/box/txn.c > index c5aaa8e0b..a831472bd 100644 > --- a/src/box/txn.c > +++ b/src/box/txn.c > @@ -484,5 +497,8 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp) > return -1; > } > txn_rollback_to_svp(txn, svp->stmt); > + if (txn->psql_txn != NULL) { > + txn->psql_txn->fk_deferred_count = svp->fk_deferred_count; > + } 7. Please, do not wrap single-line 'if' into {}. > diff --git a/test/box/misc.result b/test/box/misc.result > index 4e437d88f..840abbb66 100644 > --- a/test/box/misc.result > +++ b/test/box/misc.result > @@ -389,6 +389,7 @@ t; > - 'box.error.LOCAL_INSTANCE_ID_IS_READ_ONLY : 128' > - 'box.error.FUNCTION_EXISTS : 52' > - 'box.error.UPDATE_ARG_TYPE : 26' > + - 'box.error.FOREIGN_KEY_CONSTRAINT : 162' > - 'box.error.CROSS_ENGINE_TRANSACTION : 81' > - 'box.error.ACTION_MISMATCH : 160' > - 'box.error.FORMAT_MISMATCH_INDEX_PART : 27' > diff --git a/test/sql/transitive-transactions.result b/test/sql/transitive-transactions.result > new file mode 100644 > index 000000000..d282a5dfa > --- /dev/null > +++ b/test/sql/transitive-transactions.result > @@ -0,0 +1,124 @@ > +test_run = require('test_run').new() > +--- > +... > +test_run:cmd("setopt delimiter ';'") 8. Leading white space (git diff highlights it with red color). > +--- > +- true > +... > +-- These tests are aimed at checking transitive transactions > +-- between SQL and Lua. In particular, make sure that deferred foreign keys > +-- violations are passed correctly. > +-- > +-- box.cfg() 9. Looks like commented box.cfg that you used to run from console. > +box.begin() box.sql.execute('COMMIT'); > +--- > +... > +box.begin() box.sql.execute('ROLLBACK'); > +--- > +... > +box.sql.execute('BEGIN;') box.commit(); > +--- > +... > +box.sql.execute('BEGIN;') box.rollback(); > +--- > +... > +box.sql.execute('pragma foreign_keys = 1;'); > +--- > +... > +box.sql.execute('CREATE TABLE child(id INT PRIMARY KEY, x REFERENCES parent(y) DEFERRABLE INITIALLY DEFERRED);'); 10. Lets swap this and the next statements. AFAIK we are going to forbid not-existing table referencing, and the test will be broken. > +--- > +... > +box.sql.execute('CREATE TABLE parent(id INT PRIMARY KEY, y INT UNIQUE);'); > +--- > +... > +fk_violation_1 = function() > + box.begin() > + box.sql.execute('INSERT INTO child VALUES (1, 1);') > + box.sql.execute('COMMIT;') > +end; > +--- > +... > +fk_violation_1() box.sql.execute('ROLLBACK;') > +box.space.CHILD:select(); > +--- > +- error: 'Can not commit transaction: deferred foreign keys violations are not resolved' > +... 11. Looks like select and rollback are not executed because of failed fk_violation1. I made this: [001] -fk_violation_1() box.sql.execute('ROLLBACK;') [001] +fk_violation_1() box.sql.execute('ROLLBACK;') assert(false) And the assertion did not fail. > +fk_violation_2 = function() > + box.sql.execute('BEGIN;') > + box.sql.execute('INSERT INTO child VALUES (1, 1);') > + box.commit() > +end; > +--- > +... > +fk_violation_2() box.rollback(); > +--- > +- error: 'Can not commit transaction: deferred foreign keys violations are not resolved' 12. Same - rollback is not executed.