[tarantool-patches] Re: [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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 <tbl>" 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 <tbl>" 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.