* [tarantool-patches] [PATCH 0/4] Rework SQL transaction processing
@ 2018-05-03 18:49 Nikita Pettik
2018-05-03 18:49 ` [tarantool-patches] [PATCH 1/4] sql: remove OP_AutoCommit opcode Nikita Pettik
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Nikita Pettik @ 2018-05-03 18:49 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Branch: https://github.com/tarantool/tarantool/tree/np/rework-sql-transactions
Issue:
https://github.com/tarantool/tarantool/issues/3237
https://github.com/tarantool/tarantool/issues/3313
https://github.com/tarantool/tarantool/issues/3379
This patch-set consists of several fixes related to SQL transactions.
First patch provides slight refactoring of transactions processing
in SQL: instead of using one opcode OP_AutoCommit for BEGIN, COMMIT and
ROLLBACK handling during VDBE execution, three different opcodes are
added. Moreover, redundant switches of auto-commit flag are removed.
Now, it is set during VDBE creation and can be changed only by BEGIN
statement or by DML request (since DML must be surrounded in a separate txn).
Second patch is the main in series and introduces transitive
transactions between SQL and Lua. To implement this feature,
deferred FK counter has become attribute of transaction, and
removed from VDBE.
Last two patches fix bugs connected with SQL savepoints:
first one led to assertion fault due to wrong assert condition;
second one resulted in incorrect processing of RELEASE
statement due to obsolete SQLite code.
Nikita Pettik (4):
sql: remove OP_AutoCommit opcode
sql: allow transitive Lua <-> SQL transactions
sql: allow SAVEPOINT statement outside transaction
sql: fix SAVEPOINT RELEASE statement
src/box/errcode.h | 1 +
src/box/sql/build.c | 51 ++-----
src/box/sql/fkey.c | 66 +++------
src/box/sql/main.c | 8 +-
src/box/sql/parse.y | 11 +-
src/box/sql/pragma.c | 3 -
src/box/sql/sqliteInt.h | 33 ++++-
src/box/sql/status.c | 3 +-
src/box/sql/vdbe.c | 176 ++++++++++++-----------
src/box/sql/vdbeInt.h | 29 +---
src/box/sql/vdbeapi.c | 4 -
src/box/sql/vdbeaux.c | 83 +++++------
src/box/txn.c | 18 ++-
src/box/txn.h | 22 ++-
test/box/misc.result | 1 +
test/sql/gh-3313-savepoints-outside-txn.result | 32 +++++
test/sql/gh-3313-savepoints-outside-txn.test.lua | 18 +++
test/sql/gh-3379-release-savepoints.result | 40 ++++++
test/sql/gh-3379-release-savepoints.test.lua | 26 ++++
test/sql/transitive-transactions.result | 124 ++++++++++++++++
test/sql/transitive-transactions.test.lua | 67 +++++++++
21 files changed, 555 insertions(+), 261 deletions(-)
create mode 100644 test/sql/gh-3313-savepoints-outside-txn.result
create mode 100644 test/sql/gh-3313-savepoints-outside-txn.test.lua
create mode 100644 test/sql/gh-3379-release-savepoints.result
create mode 100644 test/sql/gh-3379-release-savepoints.test.lua
create mode 100644 test/sql/transitive-transactions.result
create mode 100644 test/sql/transitive-transactions.test.lua
--
2.15.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [tarantool-patches] [PATCH 1/4] sql: remove OP_AutoCommit opcode 2018-05-03 18:49 [tarantool-patches] [PATCH 0/4] Rework SQL transaction processing Nikita Pettik @ 2018-05-03 18:49 ` Nikita Pettik 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-03 18:49 ` [tarantool-patches] [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions Nikita Pettik ` (3 subsequent siblings) 4 siblings, 1 reply; 16+ messages in thread From: Nikita Pettik @ 2018-05-03 18:49 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik In SQLite OP_AutoCommit opcode used to set transaction operation: BEGIN, ROLLBACK and COMMIT, switching auto-commit flag in VDBE. As for Tarantool, it is confusing, since there are some differences between auto-commit modes: 'INSERT ... VALUES (1), (2), (3)' is one indivisible operation for SQLite, and three operations in real auto-commit mode for Tarantool. To simulate SQLite auto-commit mode, these three insertions are wrapped into one SEPARATE transaction, which is, in fact, not real autocommit mode. So, lets add separate explicit opcodes to BEGIN, ROLLBACK and COMMIT transactions as user's operations. Auto-commit mode is set once at VDBE creation and can be changed only by implicit opcode OP_TTransaction, which is added to each DML statement, or by 'BEGIN' SQL statement. --- src/box/sql/build.c | 51 +++++------------- src/box/sql/main.c | 3 +- src/box/sql/parse.y | 11 ++-- src/box/sql/sqliteInt.h | 31 +++++++++-- src/box/sql/vdbe.c | 136 +++++++++++++++++++++++++----------------------- src/box/sql/vdbeInt.h | 3 +- src/box/sql/vdbeapi.c | 5 +- src/box/sql/vdbeaux.c | 34 ++++++------ 8 files changed, 138 insertions(+), 136 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index a2b712a4b..2794e09cd 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3866,54 +3866,31 @@ sqlite3SrcListShiftJoinType(SrcList * p) } } -/* - * Generate VDBE code for a BEGIN statement. - */ void -sqlite3BeginTransaction(Parse * pParse, int MAYBE_UNUSED type) +sql_transaction_begin(struct Parse *parse_context) { - sqlite3 MAYBE_UNUSED *db; - Vdbe *v; - - assert(pParse != 0); - db = pParse->db; - assert(db != 0); - v = sqlite3GetVdbe(pParse); - if (!v) - return; - sqlite3VdbeAddOp0(v, OP_AutoCommit); + assert(parse_context != NULL); + struct Vdbe *v = sqlite3GetVdbe(parse_context); + if (v != NULL) + sqlite3VdbeAddOp0(v, OP_TransactionBegin); } -/* - * Generate VDBE code for a COMMIT statement. - */ void -sqlite3CommitTransaction(Parse * pParse) +sql_transaction_commit(struct Parse *parse_context) { - Vdbe *v; - - assert(pParse != 0); - assert(pParse->db != 0); - v = sqlite3GetVdbe(pParse); - if (v) { - sqlite3VdbeAddOp1(v, OP_AutoCommit, 1); - } + assert(parse_context != NULL); + struct Vdbe *v = sqlite3GetVdbe(parse_context); + if (v != NULL) + sqlite3VdbeAddOp0(v, OP_TransactionCommit); } -/* - * Generate VDBE code for a ROLLBACK statement. - */ void -sqlite3RollbackTransaction(Parse * pParse) +sql_transaction_rollback(Parse *pParse) { - Vdbe *v; - assert(pParse != 0); - assert(pParse->db != 0); - v = sqlite3GetVdbe(pParse); - if (v) { - sqlite3VdbeAddOp2(v, OP_AutoCommit, 1, 1); - } + struct Vdbe *v = sqlite3GetVdbe(pParse); + if (v != NULL) + sqlite3VdbeAddOp0(v, OP_TransactionRollback); } /* diff --git a/src/box/sql/main.c b/src/box/sql/main.c index 8e898178a..8775ef3ad 100644 --- a/src/box/sql/main.c +++ b/src/box/sql/main.c @@ -763,7 +763,6 @@ void sqlite3RollbackAll(Vdbe * pVdbe, int tripCode) { sqlite3 *db = pVdbe->db; - int inTrans = 0; (void)tripCode; struct session *user_session = current_session(); @@ -777,7 +776,7 @@ sqlite3RollbackAll(Vdbe * pVdbe, int tripCode) user_session->sql_flags &= ~SQLITE_DeferFKs; /* If one has been configured, invoke the rollback-hook callback */ - if (db->xRollbackCallback && (inTrans || !pVdbe->autoCommit)) { + if (db->xRollbackCallback && (!pVdbe->auto_commit)) { db->xRollbackCallback(db->pRollbackArg); } } diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index b078e200d..bffd065fb 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -148,16 +148,13 @@ cmdx ::= cmd. ///////////////////// Begin and end transactions. //////////////////////////// // -cmd ::= BEGIN transtype(Y) trans_opt. {sqlite3BeginTransaction(pParse, Y);} +cmd ::= BEGIN trans_opt. {sql_transaction_begin(pParse);} trans_opt ::= . trans_opt ::= TRANSACTION. trans_opt ::= TRANSACTION nm. -%type transtype {int} -transtype(A) ::= . {A = TK_DEFERRED;} -transtype(A) ::= DEFERRED(X). {A = @X; /*A-overwrites-X*/} -cmd ::= COMMIT trans_opt. {sqlite3CommitTransaction(pParse);} -cmd ::= END trans_opt. {sqlite3CommitTransaction(pParse);} -cmd ::= ROLLBACK trans_opt. {sqlite3RollbackTransaction(pParse);} +cmd ::= COMMIT trans_opt. {sql_transaction_commit(pParse);} +cmd ::= END trans_opt. {sql_transaction_commit(pParse);} +cmd ::= ROLLBACK trans_opt. {sql_transaction_rollback(pParse);} savepoint_opt ::= SAVEPOINT. savepoint_opt ::= . diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 8bb45c9d7..6bfe1352d 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3662,9 +3662,34 @@ void sqlite3PrngSaveState(void); void sqlite3PrngRestoreState(void); #endif void sqlite3RollbackAll(Vdbe *, int); -void sqlite3BeginTransaction(Parse *, int); -void sqlite3CommitTransaction(Parse *); -void sqlite3RollbackTransaction(Parse *); + +/** + * Generate opcodes which start new Tarantool transaction. + * Used from parser to process BEGIN statement. + * + * @param parse_context Current parsing context. + */ +void +sql_transaction_begin(struct Parse *parse_context); + +/** + * Generate opcodes which commit Tarantool transaction. + * Used from parser to process COMMIT statement. + * + * @param parse_context Current parsing context. + */ +void +sql_transaction_commit(struct Parse *parse_context); + +/** + * Generate opcodes which rollback Tarantool transaction. + * Used from parser to process ROLLBACK statement. + * + * @param parse_context Current parsing context. + */ +void +sql_transaction_rollback(struct Parse *parse_context); + void sqlite3Savepoint(Parse *, int, Token *); void sqlite3CloseSavepoints(Vdbe *); int sqlite3ExprIsConstant(Expr *); diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 013460f79..cf2b769c7 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2875,7 +2875,7 @@ case OP_Savepoint: { /* Assert that the p1 parameter is valid. Also that if there is no open * transaction, then there cannot be any savepoints. */ - assert(psql_txn->pSavepoint == 0 || p->autoCommit == 0); + assert(psql_txn->pSavepoint == 0 || box_txn()); assert(p1==SAVEPOINT_BEGIN||p1==SAVEPOINT_RELEASE||p1==SAVEPOINT_ROLLBACK); if (p1==SAVEPOINT_BEGIN) { @@ -2914,25 +2914,18 @@ case OP_Savepoint: { if ((rc = sqlite3VdbeCheckFk(p, 1))!=SQLITE_OK) { goto vdbe_return; } - p->autoCommit = 1; if (sqlite3VdbeHalt(p)==SQLITE_BUSY) { p->pc = (int)(pOp - aOp); - p->autoCommit = 0; p->rc = rc = SQLITE_BUSY; goto vdbe_return; } rc = p->rc; } else { - int isSchemaChange; if (p1==SAVEPOINT_ROLLBACK) { - isSchemaChange = (user_session->sql_flags & SQLITE_InternChanges)!=0; box_txn_rollback_to_savepoint(pSavepoint->tnt_savepoint); - } else { - isSchemaChange = 0; - } - if (isSchemaChange) { - sqlite3ExpirePreparedStatements(db); - user_session->sql_flags |= SQLITE_InternChanges; + if ((user_session->sql_flags & + SQLITE_InternChanges) != 0) + sqlite3ExpirePreparedStatements(db); } } @@ -2997,53 +2990,60 @@ case OP_FkCheckCommit: { break; } -/* Opcode: AutoCommit P1 P2 * * * - * - * Set the database auto-commit flag to P1 (1 or 0). If P2 is true, roll - * back any currently active transactions. If there are any active - * VMs (apart from this one), then a ROLLBACK fails. A COMMIT fails if - * there are active writing VMs or active VMs that use shared cache. - * - * This instruction causes the VM to halt. - */ -case OP_AutoCommit: { - int desiredAutoCommit; - int iRollback; - - desiredAutoCommit = pOp->p1; - iRollback = pOp->p2; - assert(desiredAutoCommit==1 || desiredAutoCommit==0); - assert(desiredAutoCommit==1 || iRollback==0); - assert(db->nVdbeActive>0); /* At least this one VM is active */ - - if (desiredAutoCommit!=p->autoCommit) { - if (iRollback) { - assert(desiredAutoCommit==1); - box_txn_rollback(); - sqlite3RollbackAll(p, SQLITE_ABORT_ROLLBACK); - p->autoCommit = 1; - } else if ((rc = sqlite3VdbeCheckFk(p, 1))!=SQLITE_OK) { - goto vdbe_return; - } else { - p->autoCommit = (u8)desiredAutoCommit; - if (p->autoCommit == 0) { - sql_txn_begin(p); - } - } - if (sqlite3VdbeHalt(p)==SQLITE_BUSY) { - p->pc = (int)(pOp - aOp); - p->autoCommit = (u8)(1-desiredAutoCommit); - p->rc = rc = SQLITE_BUSY; - goto vdbe_return; +/* Opcode: TransactionBegin * * * * * + * + * Start Tarantool's transaction. + * Only do that if there is no other active transactions. + * Otherwise, raise an error with appropriate error message. + */ +case OP_TransactionBegin: { + if (!box_txn()) { + p->auto_commit = false; + rc = sql_txn_begin(p); + if (rc != SQLITE_OK) + goto abort_due_to_error; + } else { + sqlite3VdbeError(p, "cannot start a transaction within " + "a transaction"); + rc = SQLITE_ERROR; + goto abort_due_to_error; + } + break; +} + +/* Opcode: TransactionCommit * * * * * + * + * Commit Tarantool's transaction. + * If there is no active transaction, raise an error. + */ +case OP_TransactionCommit: { + if (box_txn()) { + if (box_txn_commit() != 0) { + rc = SQL_TARANTOOL_ERROR; + goto abort_due_to_error; } - rc = SQLITE_DONE; - goto vdbe_return; } else { - sqlite3VdbeError(p, - (!desiredAutoCommit)?"cannot start a transaction within a transaction":( - (iRollback)?"cannot rollback - no transaction is active": - "cannot commit - no transaction is active")); + sqlite3VdbeError(p, "cannot commit - no transaction is active"); + rc = SQLITE_ERROR; + goto abort_due_to_error; + } + break; +} +/* Opcode: TransactionRollback * * * * * + * + * Rollback Tarantool's transaction. + * If there is no active transaction, raise an error. + */ +case OP_TransactionRollback: { + if (box_txn()) { + if (box_txn_rollback() != 0) { + rc = SQL_TARANTOOL_ERROR; + goto abort_due_to_error; + } + } else { + sqlite3VdbeError(p, "cannot rollback - no " + "transaction is active"); rc = SQLITE_ERROR; goto abort_due_to_error; } @@ -3052,18 +3052,26 @@ case OP_AutoCommit: { /* Opcode: TTransaction * * * * * * - * Start Tarantool's transaction. - * Only do that if auto commit mode is on. This should be no-op - * if this opcode was emitted inside a transaction. + * Start Tarantool's transaction, if there is no active + * transactions. Otherwise, create anonymous savepoint, + * which is used to correctly process ABORT statement inside + * outer transaction. + * + * In contrast to OP_TransactionBegin, this is service opcode, + * generated automatically alongside with DML routine. */ case OP_TTransaction: { - if (p->autoCommit) { - rc = box_txn_begin() == 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR; - } - if (box_txn() - && p->autoCommit == 0){ + if (!box_txn()) { + if (box_txn_begin() != 0) { + rc = SQL_TARANTOOL_ERROR; + goto abort_due_to_error; + } + } else { p->anonymous_savepoint = sql_savepoint(p, NULL); - + if (p->anonymous_savepoint == NULL) { + rc = SQL_TARANTOOL_ERROR; + goto abort_due_to_error; + } } break; } diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index ab9147c28..4b1767e24 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -381,7 +381,8 @@ struct Vdbe { u8 ignoreRaised; /* Flag for ON CONFLICT IGNORE for triggers */ struct sql_txn *psql_txn; /* Data related to current transaction */ - u8 autoCommit; /* The auto-commit flag. */ + /** The auto-commit flag. */ + bool auto_commit; /* * `nDeferredCons` and `nDeferredImmCons` are stored in vdbe during * vdbe execution and moved to sql_txn when it needs to be saved until diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index 6e4185924..d8cdefa7c 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -560,9 +560,8 @@ sqlite3Step(Vdbe * p) db->u1.isInterrupted = 0; } - assert(p->autoCommit == 0 - || (p->nDeferredCons == 0 && p->nDeferredImmCons == 0) - ); + assert(box_txn() || + (p->nDeferredCons == 0 && p->nDeferredImmCons == 0)); #ifndef SQLITE_OMIT_TRACE if ((db->xProfile || (db->mTrace & SQLITE_TRACE_PROFILE) != 0) diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index b3998ea27..0bd0d455b 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -65,9 +65,9 @@ sqlite3VdbeCreate(Parse * pParse) db->pVdbe = p; p->magic = VDBE_MAGIC_INIT; p->pParse = pParse; - p->autoCommit = (char)box_txn() == 0 ? 1 : 0; + p->auto_commit = !box_txn(); p->schema_ver = box_schema_version(); - if (!p->autoCommit) { + if (!p->auto_commit) { p->psql_txn = in_txn()->psql_txn; p->nDeferredCons = p->psql_txn->nDeferredConsSave; p->nDeferredImmCons = p->psql_txn->nDeferredImmConsSave; @@ -2477,31 +2477,27 @@ sqlite3VdbeCheckFk(Vdbe * p, int deferred) int sql_txn_begin(Vdbe *p) { - struct txn *ptxn; - if (in_txn()) { diag_set(ClientError, ER_ACTIVE_TRANSACTION); - return -1; + return SQL_TARANTOOL_ERROR; } - ptxn = txn_begin(false); + struct txn *ptxn = txn_begin(false); if (ptxn == NULL) - return -1; + return SQLITE_NOMEM; ptxn->psql_txn = region_alloc_object(&fiber()->gc, struct sql_txn); if (ptxn->psql_txn == NULL) { box_txn_rollback(); - return -1; + return SQLITE_NOMEM; } memset(ptxn->psql_txn, 0, sizeof(struct sql_txn)); p->psql_txn = ptxn->psql_txn; - return 0; + return SQLITE_OK; } Savepoint * -sql_savepoint(Vdbe *p, - const char *zName) +sql_savepoint(Vdbe *p, const char *zName) { assert(p); - assert(p->psql_txn); size_t nName = zName ? strlen(zName) + 1 : 0; size_t savepoint_sz = sizeof(Savepoint) + nName; Savepoint *pNew; @@ -2509,8 +2505,11 @@ sql_savepoint(Vdbe *p, pNew = (Savepoint *)region_aligned_alloc(&fiber()->gc, savepoint_sz, alignof(Savepoint)); - if (!pNew) + if (pNew == NULL) { + diag_set(OutOfMemory, savepoint_sz, "region", + "savepoint"); return NULL; + } pNew->tnt_savepoint = box_txn_savepoint(); if (!pNew->tnt_savepoint) return NULL; @@ -2595,7 +2594,7 @@ sqlite3VdbeHalt(Vdbe * p) */ if (mrc != SQLITE_INTERRUPT) { if ((mrc == SQLITE_NOMEM || mrc == SQLITE_FULL) - && p->autoCommit == 0) { + && box_txn()) { eStatementOp = SAVEPOINT_ROLLBACK; } else { /* We are forced to roll back the active transaction. Before doing @@ -2606,7 +2605,6 @@ sqlite3VdbeHalt(Vdbe * p) sqlite3RollbackAll(p, SQLITE_ABORT_ROLLBACK); sqlite3CloseSavepoints(p); - p->autoCommit = 1; p->nChange = 0; } } @@ -2623,7 +2621,7 @@ sqlite3VdbeHalt(Vdbe * p) * Note: This block also runs if one of the special errors handled * above has occurred. */ - if (p->autoCommit) { + if (p->auto_commit) { if (p->rc == SQLITE_OK || (p->errorAction == ON_CONFLICT_ACTION_FAIL && !isSpecialError)) { @@ -2682,7 +2680,6 @@ sqlite3VdbeHalt(Vdbe * p) closeCursorsAndFree(p); sqlite3RollbackAll(p, SQLITE_ABORT_ROLLBACK); sqlite3CloseSavepoints(p); - p->autoCommit = 1; p->nChange = 0; } } @@ -2706,7 +2703,6 @@ sqlite3VdbeHalt(Vdbe * p) closeCursorsAndFree(p); sqlite3RollbackAll(p, SQLITE_ABORT_ROLLBACK); sqlite3CloseSavepoints(p); - p->autoCommit = 1; p->nChange = 0; } } @@ -2742,7 +2738,7 @@ sqlite3VdbeHalt(Vdbe * p) if (!box_txn()) fiber_gc(); - assert(db->nVdbeActive > 0 || p->autoCommit == 0 || + assert(db->nVdbeActive > 0 || box_txn() || p->anonymous_savepoint == NULL); return (p->rc == SQLITE_BUSY ? SQLITE_BUSY : SQLITE_OK); } -- 2.15.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 1/4] sql: remove OP_AutoCommit opcode 2018-05-03 18:49 ` [tarantool-patches] [PATCH 1/4] sql: remove OP_AutoCommit opcode Nikita Pettik @ 2018-05-04 14:12 ` Vladislav Shpilevoy 2018-05-05 19:14 ` n.pettik 0 siblings, 1 reply; 16+ messages in thread From: Vladislav Shpilevoy @ 2018-05-04 14:12 UTC (permalink / raw) To: Nikita Pettik, tarantool-patches Hello. Thanks for contributing! See 6 comments below. 1. OP_AutoCommit still exists in mkopcodeh.sh. On 03/05/2018 21:49, Nikita Pettik wrote: > In SQLite OP_AutoCommit opcode used to set transaction operation: > BEGIN, ROLLBACK and COMMIT, switching auto-commit flag in VDBE. > As for Tarantool, it is confusing, since there are some differences > between auto-commit modes: 'INSERT ... VALUES (1), (2), (3)' is one > indivisible operation for SQLite, and three operations in real > auto-commit mode for Tarantool. To simulate SQLite auto-commit mode, > these three insertions are wrapped into one SEPARATE transaction, > which is, in fact, not real autocommit mode. > So, lets add separate explicit opcodes to BEGIN, ROLLBACK and COMMIT > transactions as user's operations. Auto-commit mode is set once at VDBE > creation and can be changed only by implicit opcode OP_TTransaction, > which is added to each DML statement, or by 'BEGIN' SQL statement. > --- > src/box/sql/build.c | 51 +++++------------- > src/box/sql/main.c | 3 +- > src/box/sql/parse.y | 11 ++-- > src/box/sql/sqliteInt.h | 31 +++++++++-- > src/box/sql/vdbe.c | 136 +++++++++++++++++++++++++----------------------- > src/box/sql/vdbeInt.h | 3 +- > src/box/sql/vdbeapi.c | 5 +- > src/box/sql/vdbeaux.c | 34 ++++++------ > 8 files changed, 138 insertions(+), 136 deletions(-) > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 013460f79..cf2b769c7 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -2875,7 +2875,7 @@ case OP_Savepoint: { > /* Assert that the p1 parameter is valid. Also that if there is no open > * transaction, then there cannot be any savepoints. > */ > - assert(psql_txn->pSavepoint == 0 || p->autoCommit == 0); > + assert(psql_txn->pSavepoint == 0 || box_txn()); 2. Please, use explicit ==/!= NULL (pSavepoint is pointer). > +/* Opcode: TransactionBegin * * * * * > + * > + * Start Tarantool's transaction. > + * Only do that if there is no other active transactions. > + * Otherwise, raise an error with appropriate error message. > + */ > +case OP_TransactionBegin: { > + if (!box_txn()) { 3. sql_txn_begin() does this check for you. Here you can always call sql_txn_begin() and check result code. After this you can remove manual error message creating, and just goto abort_due_to_error. Error code is always SQL_TARANTOOL_ERROR. > + > +/* Opcode: TransactionCommit * * * * * > + * > + * Commit Tarantool's transaction. > + * If there is no active transaction, raise an error. > + */ > +case OP_TransactionCommit: { > + if (box_txn()) { > + if (box_txn_commit() != 0) { > + rc = SQL_TARANTOOL_ERROR; > + goto abort_due_to_error; > } > - rc = SQLITE_DONE; > - goto vdbe_return; > } else { > - sqlite3VdbeError(p, > - (!desiredAutoCommit)?"cannot start a transaction within a transaction":( > - (iRollback)?"cannot rollback - no transaction is active": > - "cannot commit - no transaction is active")); > + sqlite3VdbeError(p, "cannot commit - no transaction is active"); > + rc = SQLITE_ERROR; > + goto abort_due_to_error; 4. I am not sure, that we must throw on commit/rollback with no transaction. In Lua it is ok. Possibly you should ask in the server team chat. > @@ -3052,18 +3052,26 @@ case OP_AutoCommit: { > > /* Opcode: TTransaction * * * * * > * > - * Start Tarantool's transaction. > - * Only do that if auto commit mode is on. This should be no-op > - * if this opcode was emitted inside a transaction. > + * Start Tarantool's transaction, if there is no active > + * transactions. Otherwise, create anonymous savepoint, > + * which is used to correctly process ABORT statement inside > + * outer transaction. > + * > + * In contrast to OP_TransactionBegin, this is service opcode, > + * generated automatically alongside with DML routine. > */ > case OP_TTransaction: { > - if (p->autoCommit) { > - rc = box_txn_begin() == 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR; > - } > - if (box_txn() > - && p->autoCommit == 0){ > + if (!box_txn()) { > + if (box_txn_begin() != 0) { 5. Same as 3. If you need error code, then use box_error_code() when begin() fails. > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index b3998ea27..0bd0d455b 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -2477,31 +2477,27 @@ sqlite3VdbeCheckFk(Vdbe * p, int deferred) > int > sql_txn_begin(Vdbe *p) > { > - struct txn *ptxn; > - > if (in_txn()) { > diag_set(ClientError, ER_ACTIVE_TRANSACTION); > - return -1; > + return SQL_TARANTOOL_ERROR; 6. It is step backwards to sqlite result codes style. Lets return 0/-1, and set errors via diag_set. If sql_txn_begin() returns -1, it is always SQL_TARANTOOL_ERROR. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 1/4] sql: remove OP_AutoCommit opcode 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-05-05 19:14 ` n.pettik 0 siblings, 0 replies; 16+ messages in thread From: n.pettik @ 2018-05-05 19:14 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy > 1. OP_AutoCommit still exists in mkopcodeh.sh. Ok, removed: --- a/extra/mkopcodeh.sh +++ b/extra/mkopcodeh.sh @@ -146,7 +146,6 @@ while [ "$i" -lt "$nOp" ]; do eval "name=\$ARRAY_order_$i" case "$name" in # The following are the opcodes that are processed by resolveP2Values() - OP_AutoCommit | \ >> + assert(psql_txn->pSavepoint == 0 || box_txn()); > 2. Please, use explicit ==/!= NULL (pSavepoint is pointer). --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2875,7 +2875,7 @@ case OP_Savepoint: { /* Assert that the p1 parameter is valid. Also that if there is no open * transaction, then there cannot be any savepoints. */ - assert(psql_txn->pSavepoint == 0 || box_txn()); + assert(psql_txn->pSavepoint == NULL || box_txn()); >> +/* Opcode: TransactionBegin * * * * * >> + * >> + * Start Tarantool's transaction. >> + * Only do that if there is no other active transactions. >> + * Otherwise, raise an error with appropriate error message. >> + */ >> +case OP_TransactionBegin: { >> + if (!box_txn()) { > > 3. sql_txn_begin() does this check for you. Here you can always call sql_txn_begin() and > check result code. After this you can remove manual error message creating, and just goto > abort_due_to_error. Error code is always SQL_TARANTOOL_ERROR. In fifth comment you wrote that you would better return 0/-1 from sql_txn_begin() So anyway I have to manually set return code: @@ -2997,17 +2997,9 @@ case OP_FkCheckCommit: { * Otherwise, raise an error with appropriate error message. */ case OP_TransactionBegin: { - if (!box_txn()) { - p->auto_commit = false; - rc = sql_txn_begin(p); - if (rc != SQLITE_OK) - goto abort_due_to_error; - } else { - sqlite3VdbeError(p, "cannot start a transaction within " - "a transaction"); - rc = SQLITE_ERROR; + if (sql_txn_begin(p) != 0) { + rc = SQL_TARANTOOL_ERROR; goto abort_due_to_error; } + p->auto_commit = false; break; >> + >> +/* Opcode: TransactionCommit * * * * * >> + * >> + * Commit Tarantool's transaction. >> + * If there is no active transaction, raise an error. >> + */ >> +case OP_TransactionCommit: { >> + if (box_txn()) { >> + if (box_txn_commit() != 0) { >> + rc = SQL_TARANTOOL_ERROR; >> + goto abort_due_to_error; >> } >> - rc = SQLITE_DONE; >> - goto vdbe_return; >> } else { >> - sqlite3VdbeError(p, >> - (!desiredAutoCommit)?"cannot start a transaction within a transaction":( >> - (iRollback)?"cannot rollback - no transaction is active": >> - "cannot commit - no transaction is active")); >> + sqlite3VdbeError(p, "cannot commit - no transaction is active"); >> + rc = SQLITE_ERROR; >> + goto abort_due_to_error; > > 4. I am not sure, that we must throw on commit/rollback with no > transaction. In Lua it is ok. Possibly you should ask in the server team chat. Well, I was sentenced to investigate SQL ANSI, so I found this: (2006 standard, Part 2: Foundation) 17.7 <commit statement> • If an atomic execution context is active, then an exception condition is raised: invalid transaction termination. *Atomic execution context, as I understand is sort of auto-commit mode:* • 4.33.5 SQL-statement atomicity and statement execution contexts The following are non-atomic SQL-statements: • — <call statement> • — <execute statement> • — <execute immediate statement> • — <commit statement> • — <return statement> • — <rollback statement> • — <savepoint statement> All other SQL-statements are atomic SQL-statements. The statement execution context brought into existence by the execution of a non-atomic SQL-statement is a non-atomic execution context. >> @@ -3052,18 +3052,26 @@ case OP_AutoCommit: { >> /* Opcode: TTransaction * * * * * >> * >> - * Start Tarantool's transaction. >> - * Only do that if auto commit mode is on. This should be no-op >> - * if this opcode was emitted inside a transaction. >> + * Start Tarantool's transaction, if there is no active >> + * transactions. Otherwise, create anonymous savepoint, >> + * which is used to correctly process ABORT statement inside >> + * outer transaction. >> + * >> + * In contrast to OP_TransactionBegin, this is service opcode, >> + * generated automatically alongside with DML routine. >> */ >> case OP_TTransaction: { >> - if (p->autoCommit) { >> - rc = box_txn_begin() == 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR; >> - } >> - if (box_txn() >> - && p->autoCommit == 0){ >> + if (!box_txn()) { >> + if (box_txn_begin() != 0) { > > 5. Same as 3. If you need error code, then use box_error_code() when begin() fails. To be honest, I didn’t get this idea. VDBE now supports only SQL_TARANTOOL_ERROR, which in turn results in box_error_message() displaying. Do you mean to expose VDBE error codes with enum box_error_code ? > >> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c >> index b3998ea27..0bd0d455b 100644 >> --- a/src/box/sql/vdbeaux.c >> +++ b/src/box/sql/vdbeaux.c >> @@ -2477,31 +2477,27 @@ sqlite3VdbeCheckFk(Vdbe * p, int deferred) >> int >> sql_txn_begin(Vdbe *p) >> { >> - struct txn *ptxn; >> - >> if (in_txn()) { >> diag_set(ClientError, ER_ACTIVE_TRANSACTION); >> - return -1; >> + return SQL_TARANTOOL_ERROR; > > 6. It is step backwards to sqlite result codes style. Lets return 0/-1, > and set errors via diag_set. If sql_txn_begin() returns -1, it is always > SQL_TARANTOOL_ERROR. Ok: +++ b/src/box/sql/vdbeaux.c @@ -2479,19 +2479,21 @@ sql_txn_begin(Vdbe *p) { if (in_txn()) { diag_set(ClientError, ER_ACTIVE_TRANSACTION); - return SQL_TARANTOOL_ERROR; + return -1; } struct txn *ptxn = txn_begin(false); if (ptxn == NULL) - return SQLITE_NOMEM; + return -1; ptxn->psql_txn = region_alloc_object(&fiber()->gc, struct sql_txn); if (ptxn->psql_txn == NULL) { box_txn_rollback(); - return SQLITE_NOMEM; + 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 SQLITE_OK; + return 0; } ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions 2018-05-03 18:49 [tarantool-patches] [PATCH 0/4] Rework SQL transaction processing Nikita Pettik 2018-05-03 18:49 ` [tarantool-patches] [PATCH 1/4] sql: remove OP_AutoCommit opcode Nikita Pettik @ 2018-05-03 18:49 ` Nikita Pettik 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-03 18:49 ` [tarantool-patches] [PATCH 3/4] sql: allow SAVEPOINT statement outside transaction Nikita Pettik ` (2 subsequent siblings) 4 siblings, 1 reply; 16+ messages in thread From: Nikita Pettik @ 2018-05-03 18:49 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik 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. 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 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/errcode.h b/src/box/errcode.h index e5d882df8..ba5288059 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -214,6 +214,7 @@ struct errcode_record { /*159 */_(ER_SQL_BIND_NOT_FOUND, "Parameter %s was not found in the statement") \ /*160 */_(ER_ACTION_MISMATCH, "Field %d contains %s on conflict action, but %s in index parts") \ /*161 */_(ER_VIEW_MISSING_SQL, "Space declared as a view must have SQL statement") \ + /*162 */_(ER_FOREIGN_KEY_CONSTRAINT, "Can not commit transaction: deferred foreign keys violations are not resolved") \ /* * !IMPORTANT! Please follow instructions at start of the file 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. */ void sqlite3FkDropTable(Parse * pParse, SrcList * pName, Table * pTab) @@ -774,31 +770,17 @@ sqlite3FkDropTable(Parse * pParse, SrcList * pName, Table * pTab) if ((user_session->sql_flags & SQLITE_ForeignKeys) && !space_is_view(pTab)) { - int iSkip = 0; Vdbe *v = sqlite3GetVdbe(pParse); - - assert(v); /* VDBE has already been allocated */ - if (sqlite3FkReferences(pTab) == 0) { - /* Search for a deferred foreign key constraint for which this table - * is the child table. If one cannot be found, return without - * generating any VDBE code. If one can be found, then jump over - * the entire DELETE if there are no outstanding deferred constraints - * when this statement is run. - */ - FKey *p; - for (p = pTab->pFKey; p; p = p->pNextFrom) { - if (p->isDeferred - || (user_session-> - sql_flags & SQLITE_DeferFKs)) - break; - } - if (!p) + assert(v != NULL); + /* + * Search for a foreign key constraint for which + * this table is the parent table. If one can't be + * found, return without generating any VDBE code, + * since it makes no sense starting transaction + * to test FK violations. + */ + if (sqlite3FkReferences(pTab) == NULL) return; - iSkip = sqlite3VdbeMakeLabel(v); - sqlite3VdbeAddOp2(v, OP_FkIfZero, 1, iSkip); - VdbeCoverage(v); - } - pParse->disableTriggers = 1; /* Staring new transaction before DELETE FROM <tbl> */ sqlite3VdbeAddOp0(v, OP_TTransaction); @@ -813,10 +795,6 @@ sqlite3FkDropTable(Parse * pParse, SrcList * pName, Table * pTab) * commit changes. */ sqlite3VdbeAddOp0(v, OP_FkCheckCommit); - - if (iSkip) { - sqlite3VdbeResolveLabel(v, iSkip); - } } } diff --git a/src/box/sql/main.c b/src/box/sql/main.c index 8775ef3ad..0acf7bc64 100644 --- a/src/box/sql/main.c +++ b/src/box/sql/main.c @@ -770,11 +770,6 @@ sqlite3RollbackAll(Vdbe * pVdbe, int tripCode) assert((user_session->sql_flags & SQLITE_InternChanges) == 0 || db->init.busy == 1); - /* Any deferred constraint violations have now been resolved. */ - pVdbe->nDeferredCons = 0; - pVdbe->nDeferredImmCons = 0; - user_session->sql_flags &= ~SQLITE_DeferFKs; - /* If one has been configured, invoke the rollback-hook callback */ if (db->xRollbackCallback && (!pVdbe->auto_commit)) { db->xRollbackCallback(db->pRollbackArg); diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index e41f69b02..73cd34cc0 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -320,9 +320,6 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ user_session->sql_flags |= mask; } else { user_session->sql_flags &= ~mask; - if (mask == SQLITE_DeferFKs) { - v->nDeferredImmCons = 0; - } } /* Many of the flag-pragmas modify the code diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 6bfe1352d..27d421b7e 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1849,8 +1849,6 @@ struct FuncDestructor { struct Savepoint { box_txn_savepoint_t *tnt_savepoint; /* Tarantool's savepoint struct */ char *zName; /* Savepoint name (nul-terminated) */ - i64 nDeferredCons; /* Number of deferred fk violations */ - i64 nDeferredImmCons; /* Number of deferred imm fk. */ Savepoint *pNext; /* Parent savepoint (if any) */ }; diff --git a/src/box/sql/status.c b/src/box/sql/status.c index 8742bbf8b..84f07cc68 100644 --- a/src/box/sql/status.c +++ b/src/box/sql/status.c @@ -333,8 +333,7 @@ sqlite3_db_status(sqlite3 * db, /* The database connection whose status is desir break; } const struct sql_txn *psql_txn = ptxn->psql_txn; - *pCurrent = psql_txn->nDeferredImmConsSave > 0 || - psql_txn->nDeferredConsSave > 0; + *pCurrent = psql_txn->fk_deferred_count > 0; break; } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index cf2b769c7..1192fc399 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1029,7 +1029,6 @@ case OP_Halt: { p->rc = SQLITE_BUSY; } else { assert(rc==SQLITE_OK || (p->rc&0xff)==SQLITE_CONSTRAINT); - assert(rc==SQLITE_OK || p->nDeferredCons>0 || p->nDeferredImmCons>0); rc = p->rc ? SQLITE_ERROR : SQLITE_DONE; } goto vdbe_return; @@ -2950,8 +2949,8 @@ case OP_Savepoint: { assert(pSavepoint == psql_txn->pSavepoint); psql_txn->pSavepoint = pSavepoint->pNext; } else { - p->nDeferredCons = pSavepoint->nDeferredCons; - p->nDeferredImmCons = pSavepoint->nDeferredImmCons; + p->psql_txn->fk_deferred_count = + pSavepoint->tnt_savepoint->fk_deferred_count; } } } @@ -5064,10 +5063,10 @@ case OP_Param: { /* out2 */ * statement counter is incremented (immediate foreign key constraints). */ case OP_FkCounter: { - if (user_session->sql_flags & SQLITE_DeferFKs) { - p->nDeferredImmCons += pOp->p2; - } else if (pOp->p1) { - p->nDeferredCons += pOp->p2; + if ((user_session->sql_flags & SQLITE_DeferFKs || pOp->p1 != 0) && + !p->auto_commit) { + assert(p->psql_txn != NULL); + p->psql_txn->fk_deferred_count += pOp->p2; } else { p->nFkConstraint += pOp->p2; } @@ -5087,12 +5086,14 @@ case OP_FkCounter: { * (immediate foreign key constraint violations). */ case OP_FkIfZero: { /* jump */ - if (pOp->p1) { - VdbeBranchTaken(db->nDeferredCons == 0 && db->nDeferredImmCons == 0, 2); - if (p->nDeferredCons==0 && p->nDeferredImmCons==0) goto jump_to_p2; + if ((user_session->sql_flags & SQLITE_DeferFKs || pOp->p1) && + !p->auto_commit) { + assert(p->psql_txn != NULL); + if (p->psql_txn->fk_deferred_count == 0) + goto jump_to_p2; } else { - VdbeBranchTaken(p->nFkConstraint == 0 && db->nDeferredImmCons == 0, 2); - if (p->nFkConstraint==0 && p->nDeferredImmCons==0) goto jump_to_p2; + if (p->nFkConstraint == 0) + goto jump_to_p2; } break; } diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 4b1767e24..3a907cd93 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -332,19 +332,6 @@ struct ScanStatus { char *zName; /* Name of table or index */ }; - -struct sql_txn { - Savepoint *pSavepoint; /* List of active savepoints */ - /* - * This variables transfer deferred constraints from one - * VDBE to the next in the same transaction. - * We have to do it this way because some VDBE execute ddl and do not - * have a transaction which disallows to always store this vars here. - */ - i64 nDeferredConsSave; - i64 nDeferredImmConsSave; -}; - /* * An instance of the virtual machine. This structure contains the complete * state of the virtual machine. @@ -367,8 +354,6 @@ struct Vdbe { int iStatement; /* Statement number (or 0 if has not opened stmt) */ i64 iCurrentTime; /* Value of julianday('now') for this statement */ i64 nFkConstraint; /* Number of imm. FK constraints this VM */ - i64 nStmtDefCons; /* Number of def. constraints when stmt started */ - i64 nStmtDefImmCons; /* Number of def. imm constraints when stmt started */ uint32_t schema_ver; /* Schema version at the moment of VDBE creation. */ /* @@ -379,17 +364,10 @@ struct Vdbe { * ignoreRaised variable helps to track such situations */ u8 ignoreRaised; /* Flag for ON CONFLICT IGNORE for triggers */ - - struct sql_txn *psql_txn; /* Data related to current transaction */ + /** Data related to current transaction. */ + struct sql_txn *psql_txn; /** The auto-commit flag. */ bool auto_commit; - /* - * `nDeferredCons` and `nDeferredImmCons` are stored in vdbe during - * vdbe execution and moved to sql_txn when it needs to be saved until - * execution of the next vdbe in the same transaction. - */ - i64 nDeferredCons; /* Number of deferred fk violations */ - i64 nDeferredImmCons; /* Number of deferred imm fk. */ /* When allocating a new Vdbe object, all of the fields below should be * initialized to zero or NULL diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index d8cdefa7c..32af463f6 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -560,9 +560,6 @@ sqlite3Step(Vdbe * p) db->u1.isInterrupted = 0; } - assert(box_txn() || - (p->nDeferredCons == 0 && p->nDeferredImmCons == 0)); - #ifndef SQLITE_OMIT_TRACE if ((db->xProfile || (db->mTrace & SQLITE_TRACE_PROFILE) != 0) && !db->init.busy && p->zSql) { 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; + 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; + } + txn->psql_txn->pSavepoint = NULL; + txn->psql_txn->fk_deferred_count = 0; + } + p->psql_txn = txn->psql_txn; + } else { p->psql_txn = NULL; - p->nDeferredCons = 0; - p->nDeferredImmCons = 0; } assert(pParse->aLabel == 0); assert(pParse->nLabel == 0); @@ -2439,11 +2454,8 @@ sqlite3VdbeCloseStatement(Vdbe * p, int eOp) /* * If we have an anonymous transaction opened -> perform eOp. */ - if (savepoint && eOp == SAVEPOINT_ROLLBACK) { + if (savepoint && eOp == SAVEPOINT_ROLLBACK) rc = box_txn_rollback_to_savepoint(savepoint->tnt_savepoint); - p->nDeferredCons = savepoint->nDeferredCons; - p->nDeferredImmCons = savepoint->nDeferredImmCons; - } p->anonymous_savepoint = NULL; return rc; } @@ -2462,9 +2474,9 @@ sqlite3VdbeCloseStatement(Vdbe * p, int eOp) int sqlite3VdbeCheckFk(Vdbe * p, int deferred) { - if ((deferred && (p->nDeferredCons + p->nDeferredImmCons) > 0) - || (!deferred && p->nFkConstraint > 0) - ) { + if ((deferred && p->psql_txn != NULL && + p->psql_txn->fk_deferred_count > 0) || + (!deferred && p->nFkConstraint > 0)) { p->rc = SQLITE_CONSTRAINT_FOREIGNKEY; p->errorAction = ON_CONFLICT_ACTION_ABORT; sqlite3VdbeError(p, "FOREIGN KEY constraint failed"); @@ -2516,9 +2528,7 @@ sql_savepoint(Vdbe *p, const char *zName) if (zName) { pNew->zName = (char *)&pNew[1]; memcpy(pNew->zName, zName, nName); - } - pNew->nDeferredCons = p->nDeferredCons; - pNew->nDeferredImmCons = p->nDeferredImmCons; + }; return pNew; } @@ -2540,7 +2550,6 @@ sqlite3VdbeHalt(Vdbe * p) { int rc; /* Used to store transient return codes */ sqlite3 *db = p->db; - struct session *user_session = current_session(); /* This function contains the logic that determines if a statement or * transaction will be committed or rolled back as a result of the @@ -2657,10 +2666,6 @@ sqlite3VdbeHalt(Vdbe * p) sqlite3RollbackAll(p, SQLITE_OK); p->nChange = 0; } else { - p->nDeferredCons = 0; - p->nDeferredImmCons = 0; - user_session->sql_flags &= - ~SQLITE_DeferFKs; sqlite3CommitInternalChanges(); } } else { @@ -2739,7 +2744,7 @@ sqlite3VdbeHalt(Vdbe * p) fiber_gc(); assert(db->nVdbeActive > 0 || box_txn() || - p->anonymous_savepoint == NULL); + p->anonymous_savepoint == NULL); return (p->rc == SQLITE_BUSY ? SQLITE_BUSY : SQLITE_OK); } 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 @@ -289,7 +289,18 @@ txn_commit(struct txn *txn) assert(txn == in_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 (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; + } + } /* Do transaction conflict resolving */ if (txn->engine) { if (engine_prepare(txn->engine, txn) != 0) @@ -458,6 +469,8 @@ box_txn_savepoint() } svp->stmt = stailq_last(&txn->stmts); svp->in_sub_stmt = txn->in_sub_stmt; + if (txn->psql_txn != NULL) + svp->fk_deferred_count = txn->psql_txn->fk_deferred_count; return svp; } @@ -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; + } return 0; } diff --git a/src/box/txn.h b/src/box/txn.h index 9aedd9452..00cf34cdd 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -43,7 +43,7 @@ extern "C" { /** box statistics */ extern struct rmean *rmean_box; -struct sql_txn; +struct Savepoint; struct engine; struct space; @@ -95,10 +95,30 @@ struct txn_savepoint { * an empty transaction. */ struct stailq_entry *stmt; + /** + * Each foreign key constraint is classified as either + * immediate (by default) or deferred. In autocommit mode + * they mean the same. Inside separate transaction, + * deferred FK constraints are not checked until the + * transaction tries to commit. For as long as + * a transaction is open, it is allowed to exist in a + * state violating any number of deferred FK constraints. + */ + uint32_t fk_deferred_count; }; extern double too_long_threshold; +struct sql_txn { + /** List of active SQL savepoints. */ + struct Savepoint *pSavepoint; + /** + * This variables transfer deferred constraints from one + * VDBE to the next in the same transaction. + */ + uint32_t fk_deferred_count; +}; + struct txn { /** * A sequentially growing transaction id, assigned when 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 ';'") +--- +- 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() +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);'); +--- +... +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' +... +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' +... +box.space.CHILD:select(); +--- +- [] +... +fk_violation_3 = function() + box.begin() + box.sql.execute('INSERT INTO child VALUES (1, 1);') + box.sql.execute('INSERT INTO parent VALUES (1, 1);') + box.commit() +end; +--- +... +fk_violation_3(); +--- +... +box.space.CHILD:select(); +--- +- - [1, 1] +... +box.space.PARENT:select(); +--- +- - [1, 1] +... +-- Make sure that 'PRAGMA defer_foreign_keys' works. +-- +box.sql.execute('DROP TABLE child;') +box.sql.execute('CREATE TABLE child(id INT PRIMARY KEY, x REFERENCES parent(y))') + +fk_defer = function() + box.begin() + box.sql.execute('INSERT INTO child VALUES (1, 2);') + box.sql.execute('INSERT INTO parent VALUES (2, 2);') + box.commit() +end; +--- +... +fk_defer(); +--- +- error: FOREIGN KEY constraint failed +... +box.space.CHILD:select(); +--- +- [] +... +box.space.PARENT:select(); +--- +- - [1, 1] +... +box.sql.execute('PRAGMA defer_foreign_keys = 1;') +fk_defer(); +--- +... +box.space.CHILD:select(); +--- +- - [1, 2] +... +box.space.PARENT:select(); +--- +- - [1, 1] + - [2, 2] +... +-- Cleanup +box.sql.execute('DROP TABLE child;'); +--- +... +box.sql.execute('DROP TABLE parent;'); +--- +... diff --git a/test/sql/transitive-transactions.test.lua b/test/sql/transitive-transactions.test.lua new file mode 100644 index 000000000..d6f63a660 --- /dev/null +++ b/test/sql/transitive-transactions.test.lua @@ -0,0 +1,67 @@ +test_run = require('test_run').new() +test_run:cmd("setopt delimiter ';'") + +-- 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() + +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);'); +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(); + +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(); +box.space.CHILD:select(); + +fk_violation_3 = function() + box.begin() + box.sql.execute('INSERT INTO child VALUES (1, 1);') + box.sql.execute('INSERT INTO parent VALUES (1, 1);') + box.commit() +end; +fk_violation_3(); +box.space.CHILD:select(); +box.space.PARENT:select(); + +-- Make sure that 'PRAGMA defer_foreign_keys' works. +-- +box.sql.execute('DROP TABLE child;') +box.sql.execute('CREATE TABLE child(id INT PRIMARY KEY, x REFERENCES parent(y))') + +fk_defer = function() + box.begin() + box.sql.execute('INSERT INTO child VALUES (1, 2);') + box.sql.execute('INSERT INTO parent VALUES (2, 2);') + box.commit() +end; +fk_defer(); +box.space.CHILD:select(); +box.space.PARENT:select(); +box.sql.execute('PRAGMA defer_foreign_keys = 1;') +fk_defer(); +box.space.CHILD:select(); +box.space.PARENT:select(); + +-- Cleanup +box.sql.execute('DROP TABLE child;'); +box.sql.execute('DROP TABLE parent;'); -- 2.15.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions 2018-05-03 18:49 ` [tarantool-patches] [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions Nikita Pettik @ 2018-05-04 14:12 ` Vladislav Shpilevoy 2018-05-05 19:14 ` n.pettik 0 siblings, 1 reply; 16+ messages in thread From: Vladislav Shpilevoy @ 2018-05-04 14:12 UTC (permalink / raw) To: Nikita Pettik, tarantool-patches 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-05-05 19:14 ` n.pettik 0 siblings, 0 replies; 16+ messages in thread From: n.pettik @ 2018-05-05 19:14 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy [-- Attachment #1: Type: text/plain, Size: 14491 bytes --] 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. [-- Attachment #2: 0002-sql-allow-transitive-Lua-SQL-transactions.patch --] [-- Type: application/octet-stream, Size: 28054 bytes --] From 934b0ac08de838b2325382d02330687925c9473c Mon Sep 17 00:00:00 2001 Message-Id: <934b0ac08de838b2325382d02330687925c9473c.1525533484.git.korablev@tarantool.org> In-Reply-To: <cover.1525533484.git.korablev@tarantool.org> References: <cover.1525533484.git.korablev@tarantool.org> From: Nikita Pettik <korablev@tarantool.org> Date: Thu, 26 Apr 2018 16:55:11 +0300 Subject: [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions 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 constraints as attributes of transaction, not particular VDBE. 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 alongside with rollback - that is what ANSI SQL says. Note that in SQLite rollback doesn't occur: transaction remains open untill explicit rollback or resolving all FK violations. 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/select.c | 7 +- src/box/sql/sqliteInt.h | 2 - src/box/sql/status.c | 3 +- src/box/sql/vdbe.c | 25 +++--- src/box/sql/vdbe.h | 23 ++++++ src/box/sql/vdbeInt.h | 26 +----- src/box/sql/vdbeapi.c | 3 - src/box/sql/vdbeaux.c | 79 ++++++++++++------- src/box/txn.c | 17 +++- src/box/txn.h | 22 +++++- test/box/misc.result | 1 + test/sql/transitive-transactions.result | 126 ++++++++++++++++++++++++++++++ test/sql/transitive-transactions.test.lua | 65 +++++++++++++++ 17 files changed, 345 insertions(+), 129 deletions(-) create mode 100644 test/sql/transitive-transactions.result create mode 100644 test/sql/transitive-transactions.test.lua diff --git a/src/box/errcode.h b/src/box/errcode.h index e5d882df8..ba5288059 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -214,6 +214,7 @@ struct errcode_record { /*159 */_(ER_SQL_BIND_NOT_FOUND, "Parameter %s was not found in the statement") \ /*160 */_(ER_ACTION_MISMATCH, "Field %d contains %s on conflict action, but %s in index parts") \ /*161 */_(ER_VIEW_MISSING_SQL, "Space declared as a view must have SQL statement") \ + /*162 */_(ER_FOREIGN_KEY_CONSTRAINT, "Can not commit transaction: deferred foreign keys violations are not resolved") \ /* * !IMPORTANT! Please follow instructions at start of the file 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. */ void sqlite3FkDropTable(Parse * pParse, SrcList * pName, Table * pTab) @@ -774,31 +770,17 @@ sqlite3FkDropTable(Parse * pParse, SrcList * pName, Table * pTab) if ((user_session->sql_flags & SQLITE_ForeignKeys) && !space_is_view(pTab)) { - int iSkip = 0; Vdbe *v = sqlite3GetVdbe(pParse); - - assert(v); /* VDBE has already been allocated */ - if (sqlite3FkReferences(pTab) == 0) { - /* Search for a deferred foreign key constraint for which this table - * is the child table. If one cannot be found, return without - * generating any VDBE code. If one can be found, then jump over - * the entire DELETE if there are no outstanding deferred constraints - * when this statement is run. - */ - FKey *p; - for (p = pTab->pFKey; p; p = p->pNextFrom) { - if (p->isDeferred - || (user_session-> - sql_flags & SQLITE_DeferFKs)) - break; - } - if (!p) + assert(v != NULL); + /* + * Search for a foreign key constraint for which + * this table is the parent table. If one can't be + * found, return without generating any VDBE code, + * since it makes no sense starting transaction + * to test FK violations. + */ + if (sqlite3FkReferences(pTab) == NULL) return; - iSkip = sqlite3VdbeMakeLabel(v); - sqlite3VdbeAddOp2(v, OP_FkIfZero, 1, iSkip); - VdbeCoverage(v); - } - pParse->disableTriggers = 1; /* Staring new transaction before DELETE FROM <tbl> */ sqlite3VdbeAddOp0(v, OP_TTransaction); @@ -813,10 +795,6 @@ sqlite3FkDropTable(Parse * pParse, SrcList * pName, Table * pTab) * commit changes. */ sqlite3VdbeAddOp0(v, OP_FkCheckCommit); - - if (iSkip) { - sqlite3VdbeResolveLabel(v, iSkip); - } } } diff --git a/src/box/sql/main.c b/src/box/sql/main.c index 8775ef3ad..0acf7bc64 100644 --- a/src/box/sql/main.c +++ b/src/box/sql/main.c @@ -770,11 +770,6 @@ sqlite3RollbackAll(Vdbe * pVdbe, int tripCode) assert((user_session->sql_flags & SQLITE_InternChanges) == 0 || db->init.busy == 1); - /* Any deferred constraint violations have now been resolved. */ - pVdbe->nDeferredCons = 0; - pVdbe->nDeferredImmCons = 0; - user_session->sql_flags &= ~SQLITE_DeferFKs; - /* If one has been configured, invoke the rollback-hook callback */ if (db->xRollbackCallback && (!pVdbe->auto_commit)) { db->xRollbackCallback(db->pRollbackArg); diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index e41f69b02..73cd34cc0 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -320,9 +320,6 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ user_session->sql_flags |= mask; } else { user_session->sql_flags &= ~mask; - if (mask == SQLITE_DeferFKs) { - v->nDeferredImmCons = 0; - } } /* Many of the flag-pragmas modify the code diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 5a50413a6..b8863a850 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -1994,13 +1994,16 @@ static SQLITE_NOINLINE Vdbe * 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) + return NULL; return v; } diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 6bfe1352d..27d421b7e 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1849,8 +1849,6 @@ struct FuncDestructor { struct Savepoint { box_txn_savepoint_t *tnt_savepoint; /* Tarantool's savepoint struct */ char *zName; /* Savepoint name (nul-terminated) */ - i64 nDeferredCons; /* Number of deferred fk violations */ - i64 nDeferredImmCons; /* Number of deferred imm fk. */ Savepoint *pNext; /* Parent savepoint (if any) */ }; diff --git a/src/box/sql/status.c b/src/box/sql/status.c index 8742bbf8b..84f07cc68 100644 --- a/src/box/sql/status.c +++ b/src/box/sql/status.c @@ -333,8 +333,7 @@ sqlite3_db_status(sqlite3 * db, /* The database connection whose status is desir break; } const struct sql_txn *psql_txn = ptxn->psql_txn; - *pCurrent = psql_txn->nDeferredImmConsSave > 0 || - psql_txn->nDeferredConsSave > 0; + *pCurrent = psql_txn->fk_deferred_count > 0; break; } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 43572583f..207410c7f 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1029,7 +1029,6 @@ case OP_Halt: { p->rc = SQLITE_BUSY; } else { assert(rc==SQLITE_OK || (p->rc&0xff)==SQLITE_CONSTRAINT); - assert(rc==SQLITE_OK || p->nDeferredCons>0 || p->nDeferredImmCons>0); rc = p->rc ? SQLITE_ERROR : SQLITE_DONE; } goto vdbe_return; @@ -2950,8 +2949,8 @@ case OP_Savepoint: { assert(pSavepoint == psql_txn->pSavepoint); psql_txn->pSavepoint = pSavepoint->pNext; } else { - p->nDeferredCons = pSavepoint->nDeferredCons; - p->nDeferredImmCons = pSavepoint->nDeferredImmCons; + p->psql_txn->fk_deferred_count = + pSavepoint->tnt_savepoint->fk_deferred_count; } } } @@ -5058,10 +5057,10 @@ case OP_Param: { /* out2 */ * statement counter is incremented (immediate foreign key constraints). */ case OP_FkCounter: { - if (user_session->sql_flags & SQLITE_DeferFKs) { - p->nDeferredImmCons += pOp->p2; - } else if (pOp->p1) { - p->nDeferredCons += pOp->p2; + if ((user_session->sql_flags & SQLITE_DeferFKs || pOp->p1 != 0) && + !p->auto_commit) { + assert(p->psql_txn != NULL); + p->psql_txn->fk_deferred_count += pOp->p2; } else { p->nFkConstraint += pOp->p2; } @@ -5081,12 +5080,14 @@ case OP_FkCounter: { * (immediate foreign key constraint violations). */ case OP_FkIfZero: { /* jump */ - if (pOp->p1) { - VdbeBranchTaken(db->nDeferredCons == 0 && db->nDeferredImmCons == 0, 2); - if (p->nDeferredCons==0 && p->nDeferredImmCons==0) goto jump_to_p2; + if ((user_session->sql_flags & SQLITE_DeferFKs || pOp->p1) && + !p->auto_commit) { + assert(p->psql_txn != NULL); + if (p->psql_txn->fk_deferred_count == 0) + goto jump_to_p2; } else { - VdbeBranchTaken(p->nFkConstraint == 0 && db->nDeferredImmCons == 0, 2); - if (p->nFkConstraint==0 && p->nDeferredImmCons==0) goto jump_to_p2; + if (p->nFkConstraint == 0) + goto jump_to_p2; } break; } diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h index 340ddc766..c31983f21 100644 --- a/src/box/sql/vdbe.h +++ b/src/box/sql/vdbe.h @@ -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(); + +/** + * 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 within 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/vdbeInt.h b/src/box/sql/vdbeInt.h index 4b1767e24..3a907cd93 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -332,19 +332,6 @@ struct ScanStatus { char *zName; /* Name of table or index */ }; - -struct sql_txn { - Savepoint *pSavepoint; /* List of active savepoints */ - /* - * This variables transfer deferred constraints from one - * VDBE to the next in the same transaction. - * We have to do it this way because some VDBE execute ddl and do not - * have a transaction which disallows to always store this vars here. - */ - i64 nDeferredConsSave; - i64 nDeferredImmConsSave; -}; - /* * An instance of the virtual machine. This structure contains the complete * state of the virtual machine. @@ -367,8 +354,6 @@ struct Vdbe { int iStatement; /* Statement number (or 0 if has not opened stmt) */ i64 iCurrentTime; /* Value of julianday('now') for this statement */ i64 nFkConstraint; /* Number of imm. FK constraints this VM */ - i64 nStmtDefCons; /* Number of def. constraints when stmt started */ - i64 nStmtDefImmCons; /* Number of def. imm constraints when stmt started */ uint32_t schema_ver; /* Schema version at the moment of VDBE creation. */ /* @@ -379,17 +364,10 @@ struct Vdbe { * ignoreRaised variable helps to track such situations */ u8 ignoreRaised; /* Flag for ON CONFLICT IGNORE for triggers */ - - struct sql_txn *psql_txn; /* Data related to current transaction */ + /** Data related to current transaction. */ + struct sql_txn *psql_txn; /** The auto-commit flag. */ bool auto_commit; - /* - * `nDeferredCons` and `nDeferredImmCons` are stored in vdbe during - * vdbe execution and moved to sql_txn when it needs to be saved until - * execution of the next vdbe in the same transaction. - */ - i64 nDeferredCons; /* Number of deferred fk violations */ - i64 nDeferredImmCons; /* Number of deferred imm fk. */ /* When allocating a new Vdbe object, all of the fields below should be * initialized to zero or NULL diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index d8cdefa7c..32af463f6 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -560,9 +560,6 @@ sqlite3Step(Vdbe * p) db->u1.isInterrupted = 0; } - assert(box_txn() || - (p->nDeferredCons == 0 && p->nDeferredImmCons == 0)); - #ifndef SQLITE_OMIT_TRACE if ((db->xProfile || (db->mTrace & SQLITE_TRACE_PROFILE) != 0) && !db->init.busy && p->zSql) { diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 775e1aae7..e0f665c37 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -65,17 +65,7 @@ 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{ - p->psql_txn = NULL; - p->nDeferredCons = 0; - p->nDeferredImmCons = 0; - } assert(pParse->aLabel == 0); assert(pParse->nLabel == 0); assert(pParse->nOpAlloc == 0); @@ -83,6 +73,48 @@ sqlite3VdbeCreate(Parse * pParse) return p; } +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; +} + +int +sql_vdbe_prepare(struct Vdbe *vdbe) +{ + assert(vdbe != NULL); + struct txn *txn = in_txn(); + vdbe->auto_commit = txn == NULL; + 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 = sql_alloc_txn(); + if (txn->psql_txn == NULL) { + sqlite3DbFree(vdbe->db, vdbe); + return -1; + } + } + vdbe->psql_txn = txn->psql_txn; + } else { + vdbe->psql_txn = NULL; + } + return 0; +} + /* * Change the error string stored in Vdbe.zErrMsg */ @@ -2439,11 +2471,8 @@ sqlite3VdbeCloseStatement(Vdbe * p, int eOp) /* * If we have an anonymous transaction opened -> perform eOp. */ - if (savepoint && eOp == SAVEPOINT_ROLLBACK) { + if (savepoint && eOp == SAVEPOINT_ROLLBACK) rc = box_txn_rollback_to_savepoint(savepoint->tnt_savepoint); - p->nDeferredCons = savepoint->nDeferredCons; - p->nDeferredImmCons = savepoint->nDeferredImmCons; - } p->anonymous_savepoint = NULL; return rc; } @@ -2462,9 +2491,9 @@ sqlite3VdbeCloseStatement(Vdbe * p, int eOp) int sqlite3VdbeCheckFk(Vdbe * p, int deferred) { - if ((deferred && (p->nDeferredCons + p->nDeferredImmCons) > 0) - || (!deferred && p->nFkConstraint > 0) - ) { + if ((deferred && p->psql_txn != NULL && + p->psql_txn->fk_deferred_count > 0) || + (!deferred && p->nFkConstraint > 0)) { p->rc = SQLITE_CONSTRAINT_FOREIGNKEY; p->errorAction = ON_CONFLICT_ACTION_ABORT; sqlite3VdbeError(p, "FOREIGN KEY constraint failed"); @@ -2484,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; } @@ -2518,9 +2544,7 @@ sql_savepoint(Vdbe *p, const char *zName) if (zName) { pNew->zName = (char *)&pNew[1]; memcpy(pNew->zName, zName, nName); - } - pNew->nDeferredCons = p->nDeferredCons; - pNew->nDeferredImmCons = p->nDeferredImmCons; + }; return pNew; } @@ -2542,7 +2566,6 @@ sqlite3VdbeHalt(Vdbe * p) { int rc; /* Used to store transient return codes */ sqlite3 *db = p->db; - struct session *user_session = current_session(); /* This function contains the logic that determines if a statement or * transaction will be committed or rolled back as a result of the @@ -2659,10 +2682,6 @@ sqlite3VdbeHalt(Vdbe * p) sqlite3RollbackAll(p, SQLITE_OK); p->nChange = 0; } else { - p->nDeferredCons = 0; - p->nDeferredImmCons = 0; - user_session->sql_flags &= - ~SQLITE_DeferFKs; sqlite3CommitInternalChanges(); } } else { @@ -2741,7 +2760,7 @@ sqlite3VdbeHalt(Vdbe * p) fiber_gc(); assert(db->nVdbeActive > 0 || box_txn() || - p->anonymous_savepoint == NULL); + p->anonymous_savepoint == NULL); return (p->rc == SQLITE_BUSY ? SQLITE_BUSY : SQLITE_OK); } diff --git a/src/box/txn.c b/src/box/txn.c index c5aaa8e0b..a97a04250 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -289,7 +289,18 @@ txn_commit(struct txn *txn) assert(txn == in_txn()); assert(stailq_empty(&txn->stmts) || txn->engine); - + /* + * 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); + goto fail; + } + } /* Do transaction conflict resolving */ if (txn->engine) { if (engine_prepare(txn->engine, txn) != 0) @@ -458,6 +469,8 @@ box_txn_savepoint() } svp->stmt = stailq_last(&txn->stmts); svp->in_sub_stmt = txn->in_sub_stmt; + if (txn->psql_txn != NULL) + svp->fk_deferred_count = txn->psql_txn->fk_deferred_count; return svp; } @@ -484,5 +497,7 @@ 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; return 0; } diff --git a/src/box/txn.h b/src/box/txn.h index 9aedd9452..00cf34cdd 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -43,7 +43,7 @@ extern "C" { /** box statistics */ extern struct rmean *rmean_box; -struct sql_txn; +struct Savepoint; struct engine; struct space; @@ -95,10 +95,30 @@ struct txn_savepoint { * an empty transaction. */ struct stailq_entry *stmt; + /** + * Each foreign key constraint is classified as either + * immediate (by default) or deferred. In autocommit mode + * they mean the same. Inside separate transaction, + * deferred FK constraints are not checked until the + * transaction tries to commit. For as long as + * a transaction is open, it is allowed to exist in a + * state violating any number of deferred FK constraints. + */ + uint32_t fk_deferred_count; }; extern double too_long_threshold; +struct sql_txn { + /** List of active SQL savepoints. */ + struct Savepoint *pSavepoint; + /** + * This variables transfer deferred constraints from one + * VDBE to the next in the same transaction. + */ + uint32_t fk_deferred_count; +}; + struct txn { /** * A sequentially growing transaction id, assigned when 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..37b563a17 --- /dev/null +++ b/test/sql/transitive-transactions.result @@ -0,0 +1,126 @@ +test_run = require('test_run').new() +--- +... +test_run:cmd("setopt delimiter ';'") +--- +- 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.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 parent(id INT PRIMARY KEY, y INT UNIQUE);'); +--- +... +box.sql.execute('CREATE TABLE child(id INT PRIMARY KEY, x INT REFERENCES parent(y) DEFERRABLE INITIALLY DEFERRED);'); +--- +... +fk_violation_1 = function() + box.begin() + box.sql.execute('INSERT INTO child VALUES (1, 1);') + box.sql.execute('COMMIT;') +end; +--- +... +fk_violation_1(); +--- +- error: 'Can not commit transaction: deferred foreign keys violations are not resolved' +... +box.space.CHILD:select(); +--- +- [] +... +fk_violation_2 = function() + box.sql.execute('BEGIN;') + box.sql.execute('INSERT INTO child VALUES (1, 1);') + box.commit() +end; +--- +... +fk_violation_2(); +--- +- error: 'Can not commit transaction: deferred foreign keys violations are not resolved' +... +box.space.CHILD:select(); +--- +- [] +... +fk_violation_3 = function() + box.begin() + box.sql.execute('INSERT INTO child VALUES (1, 1);') + box.sql.execute('INSERT INTO parent VALUES (1, 1);') + box.commit() +end; +--- +... +fk_violation_3(); +--- +... +box.space.CHILD:select(); +--- +- - [1, 1] +... +box.space.PARENT:select(); +--- +- - [1, 1] +... +-- Make sure that 'PRAGMA defer_foreign_keys' works. +-- +box.sql.execute('DROP TABLE child;') +box.sql.execute('CREATE TABLE child(id INT PRIMARY KEY, x INT REFERENCES parent(y))') + +fk_defer = function() + box.begin() + box.sql.execute('INSERT INTO child VALUES (1, 2);') + box.sql.execute('INSERT INTO parent VALUES (2, 2);') + box.commit() +end; +--- +... +fk_defer(); +--- +- error: FOREIGN KEY constraint failed +... +box.space.CHILD:select(); +--- +- [] +... +box.space.PARENT:select(); +--- +- - [1, 1] +... +box.sql.execute('PRAGMA defer_foreign_keys = 1;') +fk_defer(); +--- +... +box.space.CHILD:select(); +--- +- - [1, 2] +... +box.space.PARENT:select(); +--- +- - [1, 1] + - [2, 2] +... +-- Cleanup +box.sql.execute('DROP TABLE child;'); +--- +... +box.sql.execute('DROP TABLE parent;'); +--- +... diff --git a/test/sql/transitive-transactions.test.lua b/test/sql/transitive-transactions.test.lua new file mode 100644 index 000000000..14a1e8c6b --- /dev/null +++ b/test/sql/transitive-transactions.test.lua @@ -0,0 +1,65 @@ +test_run = require('test_run').new() +test_run:cmd("setopt delimiter ';'") + +-- 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.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 parent(id INT PRIMARY KEY, y INT UNIQUE);'); +box.sql.execute('CREATE TABLE child(id INT PRIMARY KEY, x INT REFERENCES parent(y) DEFERRABLE INITIALLY DEFERRED);'); + +fk_violation_1 = function() + box.begin() + box.sql.execute('INSERT INTO child VALUES (1, 1);') + box.sql.execute('COMMIT;') +end; +fk_violation_1(); +box.space.CHILD:select(); + +fk_violation_2 = function() + box.sql.execute('BEGIN;') + box.sql.execute('INSERT INTO child VALUES (1, 1);') + box.commit() +end; +fk_violation_2(); +box.space.CHILD:select(); + +fk_violation_3 = function() + box.begin() + box.sql.execute('INSERT INTO child VALUES (1, 1);') + box.sql.execute('INSERT INTO parent VALUES (1, 1);') + box.commit() +end; +fk_violation_3(); +box.space.CHILD:select(); +box.space.PARENT:select(); + +-- Make sure that 'PRAGMA defer_foreign_keys' works. +-- +box.sql.execute('DROP TABLE child;') +box.sql.execute('CREATE TABLE child(id INT PRIMARY KEY, x INT REFERENCES parent(y))') + +fk_defer = function() + box.begin() + box.sql.execute('INSERT INTO child VALUES (1, 2);') + box.sql.execute('INSERT INTO parent VALUES (2, 2);') + box.commit() +end; +fk_defer(); +box.space.CHILD:select(); +box.space.PARENT:select(); +box.sql.execute('PRAGMA defer_foreign_keys = 1;') +fk_defer(); +box.space.CHILD:select(); +box.space.PARENT:select(); + +-- Cleanup +box.sql.execute('DROP TABLE child;'); +box.sql.execute('DROP TABLE parent;'); -- 2.15.1 [-- Attachment #3: Type: text/plain, Size: 2 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] [PATCH 3/4] sql: allow SAVEPOINT statement outside transaction 2018-05-03 18:49 [tarantool-patches] [PATCH 0/4] Rework SQL transaction processing Nikita Pettik 2018-05-03 18:49 ` [tarantool-patches] [PATCH 1/4] sql: remove OP_AutoCommit opcode Nikita Pettik 2018-05-03 18:49 ` [tarantool-patches] [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions Nikita Pettik @ 2018-05-03 18:49 ` Nikita Pettik 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-03 18:49 ` [tarantool-patches] [PATCH 4/4] sql: fix SAVEPOINT RELEASE statement Nikita Pettik 2018-05-07 13:31 ` [tarantool-patches] Re: [PATCH 0/4] Rework SQL transaction processing Vladislav Shpilevoy 4 siblings, 1 reply; 16+ messages in thread From: Nikita Pettik @ 2018-05-03 18:49 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Before this patch, usage of SAVEPOINT statement outside transaction or inside transaction started in Lua, led to assertion fault. Now, failed assert is substituted with checks to test transaction status. Closes #3313 --- src/box/sql/vdbe.c | 8 +++++- test/sql/gh-3313-savepoints-outside-txn.result | 32 ++++++++++++++++++++++++ test/sql/gh-3313-savepoints-outside-txn.test.lua | 18 +++++++++++++ 3 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 test/sql/gh-3313-savepoints-outside-txn.result create mode 100644 test/sql/gh-3313-savepoints-outside-txn.test.lua diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 1192fc399..6ea04901c 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2867,7 +2867,13 @@ case OP_Savepoint: { Savepoint *pTmp; struct sql_txn *psql_txn = p->psql_txn; - assert(psql_txn); + if (psql_txn == NULL) { + assert(!box_txn()); + sqlite3VdbeError(p, "cannot process savepoint: " + "there is no active transaction"); + rc = SQLITE_ERROR; + goto abort_due_to_error; + } p1 = pOp->p1; zName = pOp->p4.z; diff --git a/test/sql/gh-3313-savepoints-outside-txn.result b/test/sql/gh-3313-savepoints-outside-txn.result new file mode 100644 index 000000000..702d3e815 --- /dev/null +++ b/test/sql/gh-3313-savepoints-outside-txn.result @@ -0,0 +1,32 @@ +test_run = require('test_run').new() +--- +... +test_run:cmd("setopt delimiter ';'") +--- +- true +... +-- These tests check that SQL savepoints properly work outside +-- transactions as well as inside transactions started in Lua. +-- +-- box.cfg() +box.sql.execute('SAVEPOINT t1;'); +--- +- error: 'cannot process savepoint: there is no active transaction' +... +box.sql.execute('RELEASE SAVEPOINT t1;'); +--- +- error: 'cannot process savepoint: there is no active transaction' +... +box.sql.execute('ROLLBACK TO SAVEPOINT t1;'); +--- +- error: 'cannot process savepoint: there is no active transaction' +... +box.begin() box.sql.execute('SAVEPOINT t1;') box.sql.execute('RELEASE SAVEPOINT t1;') box.commit(); +--- +... +box.begin() box.sql.execute('SAVEPOINT t1;') box.sql.execute('ROLLBACK TO t1;') box.commit(); +--- +... +box.begin() box.sql.execute('SAVEPOINT t1;') box.commit(); +--- +... diff --git a/test/sql/gh-3313-savepoints-outside-txn.test.lua b/test/sql/gh-3313-savepoints-outside-txn.test.lua new file mode 100644 index 000000000..61dd9a86b --- /dev/null +++ b/test/sql/gh-3313-savepoints-outside-txn.test.lua @@ -0,0 +1,18 @@ +test_run = require('test_run').new() +test_run:cmd("setopt delimiter ';'") + +-- These tests check that SQL savepoints properly work outside +-- transactions as well as inside transactions started in Lua. +-- + +-- box.cfg() + +box.sql.execute('SAVEPOINT t1;'); +box.sql.execute('RELEASE SAVEPOINT t1;'); +box.sql.execute('ROLLBACK TO SAVEPOINT t1;'); + +box.begin() box.sql.execute('SAVEPOINT t1;') box.sql.execute('RELEASE SAVEPOINT t1;') box.commit(); + +box.begin() box.sql.execute('SAVEPOINT t1;') box.sql.execute('ROLLBACK TO t1;') box.commit(); + +box.begin() box.sql.execute('SAVEPOINT t1;') box.commit(); -- 2.15.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 3/4] sql: allow SAVEPOINT statement outside transaction 2018-05-03 18:49 ` [tarantool-patches] [PATCH 3/4] sql: allow SAVEPOINT statement outside transaction Nikita Pettik @ 2018-05-04 14:12 ` Vladislav Shpilevoy 2018-05-05 19:15 ` n.pettik 0 siblings, 1 reply; 16+ messages in thread From: Vladislav Shpilevoy @ 2018-05-04 14:12 UTC (permalink / raw) To: Nikita Pettik, tarantool-patches Hello. Thanks for contributing! See below 3 comments. On 03/05/2018 21:49, Nikita Pettik wrote: > Before this patch, usage of SAVEPOINT statement outside transaction or > inside transaction started in Lua, led to assertion fault. > Now, failed assert is substituted with checks to test transaction status. > > Closes #3313 > --- > src/box/sql/vdbe.c | 8 +++++- > test/sql/gh-3313-savepoints-outside-txn.result | 32 ++++++++++++++++++++++++ > test/sql/gh-3313-savepoints-outside-txn.test.lua | 18 +++++++++++++ > 3 files changed, 57 insertions(+), 1 deletion(-) > create mode 100644 test/sql/gh-3313-savepoints-outside-txn.result > create mode 100644 test/sql/gh-3313-savepoints-outside-txn.test.lua > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 1192fc399..6ea04901c 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -2867,7 +2867,13 @@ case OP_Savepoint: { > Savepoint *pTmp; > struct sql_txn *psql_txn = p->psql_txn; > > - assert(psql_txn); > + if (psql_txn == NULL) { > + assert(!box_txn()); > + sqlite3VdbeError(p, "cannot process savepoint: " > + "there is no active transaction"); > + rc = SQLITE_ERROR; > + goto abort_due_to_error; > + } 1. Lets use diag_set(ClientError, ER_SAVEPOINT_NO_TRANSACTION) and return TARANTOOL_ERROR. It must behave like box.savepoint. > diff --git a/test/sql/gh-3313-savepoints-outside-txn.result b/test/sql/gh-3313-savepoints-outside-txn.result > new file mode 100644 > index 000000000..702d3e815 > --- /dev/null > +++ b/test/sql/gh-3313-savepoints-outside-txn.result > @@ -0,0 +1,32 @@ > +test_run = require('test_run').new() > +--- > +... > +test_run:cmd("setopt delimiter ';'") 2. Why do you need here a special delimiter? Your test does not contain multi-line statements. > +--- > +- true > +... > +-- These tests check that SQL savepoints properly work outside > +-- transactions as well as inside transactions started in Lua. > +-- > +-- box.cfg() 3. Garbage comment. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 3/4] sql: allow SAVEPOINT statement outside transaction 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-05-05 19:15 ` n.pettik 0 siblings, 0 replies; 16+ messages in thread From: n.pettik @ 2018-05-05 19:15 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy > 1. Lets use diag_set(ClientError, ER_SAVEPOINT_NO_TRANSACTION) and return TARANTOOL_ERROR. > It must behave like box.savepoint. Ok: @@ -2869,9 +2869,8 @@ case OP_Savepoint: { if (psql_txn == NULL) { assert(!box_txn()); - sqlite3VdbeError(p, "cannot process savepoint: " - "there is no active transaction"); - rc = SQLITE_ERROR; + diag_set(ClientError, ER_SAVEPOINT_NO_TRANSACTION); + rc = SQL_TARANTOOL_ERROR; >> diff --git a/test/sql/gh-3313-savepoints-outside-txn.result b/test/sql/gh-3313-savepoints-outside-txn.result >> new file mode 100644 >> index 000000000..702d3e815 >> --- /dev/null >> +++ b/test/sql/gh-3313-savepoints-outside-txn.result >> @@ -0,0 +1,32 @@ >> +test_run = require('test_run').new() >> +--- >> +... >> +test_run:cmd("setopt delimiter ';'") > > 2. Why do you need here a special delimiter? Your test does not > contain multi-line statements. Sorry, it is garbage after copying files. Removed. >> +--- >> +- true >> +... >> +-- These tests check that SQL savepoints properly work outside >> +-- transactions as well as inside transactions started in Lua. >> +-- >> +-- box.cfg() > > 3. Garbage comment. Removed. Also, taking into consideration comment from review of last patch, I have renamed test file and merge tests from both tickets. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] [PATCH 4/4] sql: fix SAVEPOINT RELEASE statement 2018-05-03 18:49 [tarantool-patches] [PATCH 0/4] Rework SQL transaction processing Nikita Pettik ` (2 preceding siblings ...) 2018-05-03 18:49 ` [tarantool-patches] [PATCH 3/4] sql: allow SAVEPOINT statement outside transaction Nikita Pettik @ 2018-05-03 18:49 ` Nikita Pettik 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-07 13:31 ` [tarantool-patches] Re: [PATCH 0/4] Rework SQL transaction processing Vladislav Shpilevoy 4 siblings, 1 reply; 16+ messages in thread From: Nikita Pettik @ 2018-05-03 18:49 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Before this patch SAVEPOINT RELEASE statement always raised error, due to SQLite's obsolete code. Now it has been removed, and SAVEPOINT RELEASE works as desired. Closes #3379 --- src/box/sql/vdbe.c | 7 ----- test/sql/gh-3379-release-savepoints.result | 40 ++++++++++++++++++++++++++++ test/sql/gh-3379-release-savepoints.test.lua | 26 ++++++++++++++++++ 3 files changed, 66 insertions(+), 7 deletions(-) create mode 100644 test/sql/gh-3379-release-savepoints.result create mode 100644 test/sql/gh-3379-release-savepoints.test.lua diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 6ea04901c..62f393de4 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2901,13 +2901,6 @@ case OP_Savepoint: { if (!pSavepoint) { sqlite3VdbeError(p, "no such savepoint: %s", zName); rc = SQLITE_ERROR; - } else if (p1==SAVEPOINT_RELEASE) { - /* It is not possible to release (commit) a savepoint if there are - * active write statements. - */ - sqlite3VdbeError(p, "cannot release savepoint - " - "SQL statements in progress"); - rc = SQLITE_BUSY; } else { /* Determine whether or not this is a transaction savepoint. If so, diff --git a/test/sql/gh-3379-release-savepoints.result b/test/sql/gh-3379-release-savepoints.result new file mode 100644 index 000000000..5f5804b66 --- /dev/null +++ b/test/sql/gh-3379-release-savepoints.result @@ -0,0 +1,40 @@ +test_run = require('test_run').new() +--- +... +test_run:cmd("setopt delimiter ';'") +--- +- true +... +-- These tests check that release of SQL savepoints works as desired. +-- +-- box.cfg() +release_sv = function() + box.begin() + box.sql.execute('SAVEPOINT t1;') + box.sql.execute('RELEASE SAVEPOINT t1;') +end; +--- +... +release_sv(); +--- +... +box.commit(); +--- +... +release_sv_fail = function() + box.begin() + box.sql.execute('SAVEPOINT t1;') + box.sql.execute('SAVEPOINT t2;') + box.sql.execute('RELEASE SAVEPOINT t2;') + box.sql.execute('RELEASE SAVEPOINT t1;') + box.sql.execute('ROLLBACK TO t1;') +end; +--- +... +release_sv_fail(); +--- +- error: 'no such savepoint: T1' +... +box.commit(); +--- +... diff --git a/test/sql/gh-3379-release-savepoints.test.lua b/test/sql/gh-3379-release-savepoints.test.lua new file mode 100644 index 000000000..6b299045d --- /dev/null +++ b/test/sql/gh-3379-release-savepoints.test.lua @@ -0,0 +1,26 @@ +test_run = require('test_run').new() +test_run:cmd("setopt delimiter ';'") + +-- These tests check that release of SQL savepoints works as desired. +-- + +-- box.cfg() + +release_sv = function() + box.begin() + box.sql.execute('SAVEPOINT t1;') + box.sql.execute('RELEASE SAVEPOINT t1;') +end; +release_sv(); +box.commit(); + +release_sv_fail = function() + box.begin() + box.sql.execute('SAVEPOINT t1;') + box.sql.execute('SAVEPOINT t2;') + box.sql.execute('RELEASE SAVEPOINT t2;') + box.sql.execute('RELEASE SAVEPOINT t1;') + box.sql.execute('ROLLBACK TO t1;') +end; +release_sv_fail(); +box.commit(); -- 2.15.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 4/4] sql: fix SAVEPOINT RELEASE statement 2018-05-03 18:49 ` [tarantool-patches] [PATCH 4/4] sql: fix SAVEPOINT RELEASE statement Nikita Pettik @ 2018-05-04 14:12 ` Vladislav Shpilevoy 2018-05-05 19:16 ` n.pettik 0 siblings, 1 reply; 16+ messages in thread From: Vladislav Shpilevoy @ 2018-05-04 14:12 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Hello. Thanks for contributing! See below 2 comments. 1. Lets do not create a new test file on each issue. On 03/05/2018 21:49, Nikita Pettik wrote: > Before this patch SAVEPOINT RELEASE statement always raised error, > due to SQLite's obsolete code. Now it has been removed, and > SAVEPOINT RELEASE works as desired. > > Closes #3379 > --- > src/box/sql/vdbe.c | 7 ----- > test/sql/gh-3379-release-savepoints.result | 40 ++++++++++++++++++++++++++++ > test/sql/gh-3379-release-savepoints.test.lua | 26 ++++++++++++++++++ > 3 files changed, 66 insertions(+), 7 deletions(-) > create mode 100644 test/sql/gh-3379-release-savepoints.result > create mode 100644 test/sql/gh-3379-release-savepoints.test.lua > > diff --git a/test/sql/gh-3379-release-savepoints.result b/test/sql/gh-3379-release-savepoints.result > new file mode 100644 > index 000000000..5f5804b66 > --- /dev/null > +++ b/test/sql/gh-3379-release-savepoints.result > @@ -0,0 +1,40 @@ > +test_run = require('test_run').new() > +--- > +... > +test_run:cmd("setopt delimiter ';'") > +--- > +- true > +... > +-- These tests check that release of SQL savepoints works as desired. > +-- > +-- box.cfg() 2. Garbage diff. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 4/4] sql: fix SAVEPOINT RELEASE statement 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-05-05 19:16 ` n.pettik 0 siblings, 0 replies; 16+ messages in thread From: n.pettik @ 2018-05-05 19:16 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy > 1. Lets do not create a new test file on each issue. Ok, merged tests from previous patch with tests from this one. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 0/4] Rework SQL transaction processing 2018-05-03 18:49 [tarantool-patches] [PATCH 0/4] Rework SQL transaction processing Nikita Pettik ` (3 preceding siblings ...) 2018-05-03 18:49 ` [tarantool-patches] [PATCH 4/4] sql: fix SAVEPOINT RELEASE statement Nikita Pettik @ 2018-05-07 13:31 ` Vladislav Shpilevoy 2018-05-11 7:17 ` Kirill Yukhin 2018-05-11 10:08 ` Kirill Yukhin 4 siblings, 2 replies; 16+ messages in thread From: Vladislav Shpilevoy @ 2018-05-07 13:31 UTC (permalink / raw) To: Nikita Pettik, tarantool-patches Hello. Thanks for review fixes! The whole patchset LGTM now. On 03/05/2018 21:49, Nikita Pettik wrote: > Branch: https://github.com/tarantool/tarantool/tree/np/rework-sql-transactions > Issue: > https://github.com/tarantool/tarantool/issues/3237 > https://github.com/tarantool/tarantool/issues/3313 > https://github.com/tarantool/tarantool/issues/3379 > > > This patch-set consists of several fixes related to SQL transactions. > > First patch provides slight refactoring of transactions processing > in SQL: instead of using one opcode OP_AutoCommit for BEGIN, COMMIT and > ROLLBACK handling during VDBE execution, three different opcodes are > added. Moreover, redundant switches of auto-commit flag are removed. > Now, it is set during VDBE creation and can be changed only by BEGIN > statement or by DML request (since DML must be surrounded in a separate txn). > > Second patch is the main in series and introduces transitive > transactions between SQL and Lua. To implement this feature, > deferred FK counter has become attribute of transaction, and > removed from VDBE. > > Last two patches fix bugs connected with SQL savepoints: > first one led to assertion fault due to wrong assert condition; > second one resulted in incorrect processing of RELEASE > statement due to obsolete SQLite code. > > Nikita Pettik (4): > sql: remove OP_AutoCommit opcode > sql: allow transitive Lua <-> SQL transactions > sql: allow SAVEPOINT statement outside transaction > sql: fix SAVEPOINT RELEASE statement > > src/box/errcode.h | 1 + > src/box/sql/build.c | 51 ++----- > src/box/sql/fkey.c | 66 +++------ > src/box/sql/main.c | 8 +- > src/box/sql/parse.y | 11 +- > src/box/sql/pragma.c | 3 - > src/box/sql/sqliteInt.h | 33 ++++- > src/box/sql/status.c | 3 +- > src/box/sql/vdbe.c | 176 ++++++++++++----------- > src/box/sql/vdbeInt.h | 29 +--- > src/box/sql/vdbeapi.c | 4 - > src/box/sql/vdbeaux.c | 83 +++++------ > src/box/txn.c | 18 ++- > src/box/txn.h | 22 ++- > test/box/misc.result | 1 + > test/sql/gh-3313-savepoints-outside-txn.result | 32 +++++ > test/sql/gh-3313-savepoints-outside-txn.test.lua | 18 +++ > test/sql/gh-3379-release-savepoints.result | 40 ++++++ > test/sql/gh-3379-release-savepoints.test.lua | 26 ++++ > test/sql/transitive-transactions.result | 124 ++++++++++++++++ > test/sql/transitive-transactions.test.lua | 67 +++++++++ > 21 files changed, 555 insertions(+), 261 deletions(-) > create mode 100644 test/sql/gh-3313-savepoints-outside-txn.result > create mode 100644 test/sql/gh-3313-savepoints-outside-txn.test.lua > create mode 100644 test/sql/gh-3379-release-savepoints.result > create mode 100644 test/sql/gh-3379-release-savepoints.test.lua > create mode 100644 test/sql/transitive-transactions.result > create mode 100644 test/sql/transitive-transactions.test.lua > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 0/4] Rework SQL transaction processing 2018-05-07 13:31 ` [tarantool-patches] Re: [PATCH 0/4] Rework SQL transaction processing Vladislav Shpilevoy @ 2018-05-11 7:17 ` Kirill Yukhin 2018-05-11 10:08 ` Kirill Yukhin 1 sibling, 0 replies; 16+ messages in thread From: Kirill Yukhin @ 2018-05-11 7:17 UTC (permalink / raw) To: tarantool-patches; +Cc: Nikita Pettik Hello Nikita, Vlad, On 07 мая 16:31, Vladislav Shpilevoy wrote: > Hello. Thanks for review fixes! The whole patchset LGTM now. > > On 03/05/2018 21:49, Nikita Pettik wrote: > > Branch: https://github.com/tarantool/tarantool/tree/np/rework-sql-transactions > > Issue: > > https://github.com/tarantool/tarantool/issues/3237 > > https://github.com/tarantool/tarantool/issues/3313 > > https://github.com/tarantool/tarantool/issues/3379 The patch set fails testing: [026] sql/transitive-transactions.test.lua [ fail ] [026] [026] Test failed! Result content mismatch: [033] sql/drop-table.test.lua [ pass ] [026] --- sql/transitive-transactions.result Fri May 11 10:15:28 2018 [026] +++ sql/transitive-transactions.reject Fri May 11 10:15:53 2018 [026] @@ -107,19 +107,20 @@ [026] box.sql.execute('PRAGMA defer_foreign_keys = 1;') [026] fk_defer(); [026] --- [026] +- error: 'Operation is not permitted when there is an active transaction ' [026] ... [026] box.space.CHILD:select(); [026] --- [026] -- - [1, 2] [026] +- [] [026] ... -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 0/4] Rework SQL transaction processing 2018-05-07 13:31 ` [tarantool-patches] Re: [PATCH 0/4] Rework SQL transaction processing Vladislav Shpilevoy 2018-05-11 7:17 ` Kirill Yukhin @ 2018-05-11 10:08 ` Kirill Yukhin 1 sibling, 0 replies; 16+ messages in thread From: Kirill Yukhin @ 2018-05-11 10:08 UTC (permalink / raw) To: tarantool-patches; +Cc: Nikita Pettik Hello, On 07 мая 16:31, Vladislav Shpilevoy wrote: > Hello. Thanks for review fixes! The whole patchset LGTM now. I've checked your patcheset in to 2.0 branch. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-05-11 10:08 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-03 18:49 [tarantool-patches] [PATCH 0/4] Rework SQL transaction processing Nikita Pettik 2018-05-03 18:49 ` [tarantool-patches] [PATCH 1/4] sql: remove OP_AutoCommit opcode Nikita Pettik 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-05 19:14 ` n.pettik 2018-05-03 18:49 ` [tarantool-patches] [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions Nikita Pettik 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-05 19:14 ` n.pettik 2018-05-03 18:49 ` [tarantool-patches] [PATCH 3/4] sql: allow SAVEPOINT statement outside transaction Nikita Pettik 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-05 19:15 ` n.pettik 2018-05-03 18:49 ` [tarantool-patches] [PATCH 4/4] sql: fix SAVEPOINT RELEASE statement Nikita Pettik 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-05 19:16 ` n.pettik 2018-05-07 13:31 ` [tarantool-patches] Re: [PATCH 0/4] Rework SQL transaction processing Vladislav Shpilevoy 2018-05-11 7:17 ` Kirill Yukhin 2018-05-11 10:08 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox