[tarantool-patches] Re: [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions
n.pettik
korablev at tarantool.org
Sat May 5 22:14:36 MSK 2018
Except for your comments, I also have changed behaviour of
commit statement with existing deferred FK violations.
(I started to read ANSI SQL and some things are different from SQLite ones)
According to ANSI SQL failed commit must always rollback:
17.7 <commit statement> (2006 standard, part 2: Foundation)
If any constraint is not satisfied, then any changes to SQL-data or schemas that were
made by the current SQL-transaction are canceled and an exception condition is raised:
transaction rollback — integrity constraint violation.
4.17.2 Checking of constraints (2006 standard, part 2: Foundation)
When a <commit statement> is executed, all constraints are effectively checked and,
if any constraint is not satisfied, then an exception condition is raised and
the SQL-transaction is terminated by an implicit <rollback statement>.
@@ -290,15 +290,15 @@ txn_commit(struct txn *txn)
assert(stailq_empty(&txn->stmts) || txn->engine);
/*
- * If transaction has been started in SQL, foreign key
- * constraints must not be violated. If not so, don't
- * rollback transaction, just prevent from commiting.
+ * If transaction has been started in SQL, deferred
+ * foreign key constraints must not be violated.
+ * If not so, just rollback transaction.
*/
if (txn->psql_txn != NULL) {
struct sql_txn *sql_txn = txn->psql_txn;
if (sql_txn->fk_deferred_count != 0) {
diag_set(ClientError, ER_FOREIGN_KEY_CONSTRAINT);
- return -1;
+ goto fail;
}
>> foreign keys contrains as attributes of transaction, not particular VDBE.
>
> 1. contrains -> constraints.
Fixed.
>> transaction becomes open untill all deferred FK violations are resolved
>
> 2. untill -> until.
Fixed.
>> -/*
>> - * 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?
Because deferred constraints make sense only within active transaction.
On the other hand, Tarantool doesn’t support DDL inside transaction.
Thus, anyway it would lead to execution error, so I just removed excess check.
>> + p->auto_commit = txn == NULL ? true : false;
>
> 4. txn == NULL is enough. The result is boolean already.
- p->auto_commit = txn == NULL ? true : false;
+ p->auto_commit = txn == NULL;
>
> 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?
allocVdbe(Parse * pParse)
{
Vdbe *v = pParse->pVdbe = sqlite3VdbeCreate(pParse);
- if (v)
- sqlite3VdbeAddOp2(v, OP_Init, 0, 1);
+ if (v == NULL)
+ return NULL;
+ sqlite3VdbeAddOp2(v, OP_Init, 0, 1);
if (pParse->pToplevel == 0
&& OptimizationEnabled(pParse->db, SQLITE_FactorOutConst)
) {
pParse->okConstFactor = 1;
}
+ if (sql_vdbe_prepare(v) != 0) {
+ sqlite3DbFree(pParse->db, v);
+ return NULL;
+ }
return v;
}
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
+
+/**
+ * Prepare given VDBE to execution: initialize structs connected
+ * with transaction routine: autocommit mode, deferred foreign
+ * keys counter, struct representing SQL savepoint.
+ * If execution context is already withing active transaction,
+ * just transfer transaction data to VDBE.
+ *
+ * @param vdbe VDBE to be prepared.
+ * @retval -1 on out of memory, 0 otherwise.
+ */
+int
+sql_vdbe_prepare(struct Vdbe *vdbe);
+
int sqlite3VdbeAddOp0(Vdbe *, int);
int sqlite3VdbeAddOp1(Vdbe *, int, int);
int sqlite3VdbeAddOp2(Vdbe *, int, int, int);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 219b107eb..19953285a 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -66,8 +66,34 @@ sqlite3VdbeCreate(Parse * pParse)
p->magic = VDBE_MAGIC_INIT;
p->pParse = pParse;
p->schema_ver = box_schema_version();
+ assert(pParse->aLabel == 0);
+ assert(pParse->nLabel == 0);
+ assert(pParse->nOpAlloc == 0);
+ assert(pParse->szOpAlloc == 0);
+ return p;
+}
+int
+sql_vdbe_prepare(struct Vdbe *vdbe)
+{
+ assert(vdbe != NULL);
struct txn *txn = in_txn();
- p->auto_commit = txn == NULL ? true : false;
+ vdbe->auto_commit = txn == NULL;
if (txn != NULL) {
/*
* If transaction has been started in Lua, then
@@ -76,26 +102,17 @@ sqlite3VdbeCreate(Parse * pParse)
* check FK violations, at least now.
*/
if (txn->psql_txn == NULL) {
- txn->psql_txn = region_alloc_object(&fiber()->gc,
- struct sql_txn);
+ txn->psql_txn = sql_alloc_txn();
if (txn->psql_txn == NULL)
- sqlite3DbFree(db, p);
- diag_set(OutOfMemory, sizeof(struct sql_txn),
- "region", "struct sql_txn");
- return NULL;
+ return -1;
}
- txn->psql_txn->pSavepoint = NULL;
- txn->psql_txn->fk_deferred_count = 0;
}
- p->psql_txn = txn->psql_txn;
+ vdbe->psql_txn = txn->psql_txn;
} else {
- p->psql_txn = NULL;
+ vdbe->psql_txn = NULL;
}
- assert(pParse->aLabel == 0);
- assert(pParse->nLabel == 0);
- assert(pParse->nOpAlloc == 0);
- assert(pParse->szOpAlloc == 0);
- return p;
+ return 0;
}
/*
@@ -2496,14 +2513,11 @@ sql_txn_begin(Vdbe *p)
struct txn *ptxn = txn_begin(false);
if (ptxn == NULL)
return -1;
- ptxn->psql_txn = region_alloc_object(&fiber()->gc, struct sql_txn);
+ ptxn->psql_txn = sql_alloc_txn();
if (ptxn->psql_txn == NULL) {
box_txn_rollback();
- diag_set(OutOfMemory, sizeof(struct sql_txn), "region",
- "struct sql_txn");
return -1;
}
- memset(ptxn->psql_txn, 0, sizeof(struct sql_txn));
p->psql_txn = ptxn->psql_txn;
return 0;
}
>
>> + 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.
As you wish:
+
+struct sql_txn *
+sql_alloc_txn()
+{
+ struct sql_txn *txn = region_alloc_object(&fiber()->gc,
+ struct sql_txn);
+ if (txn == NULL) {
+ diag_set(OutOfMemory, sizeof(struct sql_txn), "region",
+ "struct sql_txn");
+ return NULL;
+ }
+ txn->pSavepoint = NULL;
+ txn->fk_deferred_count = 0;
+ return txn;
+}
@@ -2496,14 +2513,11 @@ sql_txn_begin(Vdbe *p)
struct txn *ptxn = txn_begin(false);
if (ptxn == NULL)
return -1;
- ptxn->psql_txn = region_alloc_object(&fiber()->gc, struct sql_txn);
+ ptxn->psql_txn = sql_alloc_txn();
if (ptxn->psql_txn == NULL) {
box_txn_rollback();
- diag_set(OutOfMemory, sizeof(struct sql_txn), "region",
- "struct sql_txn");
return -1;
}
- memset(ptxn->psql_txn, 0, sizeof(struct sql_txn));
p->psql_txn = ptxn->psql_txn;
return 0;
}
@@ -191,6 +191,29 @@ typedef struct VdbeOpList VdbeOpList;
* for a description of what each of these routines does.
*/
Vdbe *sqlite3VdbeCreate(Parse *);
+
+/**
+ * Allocate and initialize SQL-specific struct which completes
+ * original Tarantool's txn struct using region allocator.
+ *
+ * @retval NULL on OOM, new psql_txn struct on success.
+ **/
+struct sql_txn *
+sql_alloc_txn();
Also, see diff connected with sql_vdbe_prepare().
>> 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 {}.
Sorry.
- if (txn->psql_txn != NULL) {
+ if (txn->psql_txn != NULL)
txn->psql_txn->fk_deferred_count = svp->fk_deferred_count;
- }
>> 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).
Removed.
>
>> +---
>> +- 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.
Removed.
>
>> +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.
Swapped.
>
>> +---
>> +...
>> +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.
Since I have changed behaviour (rollback instead of commit failure),
I just removed these rollbacks.
I am also attaching complete patch, since
some of diffs seem to be inconvenient to read.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sql-allow-transitive-Lua-SQL-transactions.patch
Type: application/octet-stream
Size: 27206 bytes
Desc: not available
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180505/bd09b948/attachment.obj>
-------------- next part --------------
More information about the Tarantool-patches
mailing list