From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 17F922448E for ; Thu, 15 Aug 2019 07:04:03 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id apP3QHVR94j2 for ; Thu, 15 Aug 2019 07:04:02 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 7A83323772 for ; Thu, 15 Aug 2019 07:04:02 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn From: "n.pettik" In-Reply-To: Date: Thu, 15 Aug 2019 14:04:00 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <97A342BD-97BD-451F-A54B-A9CF18D352A3@tarantool.org> References: <7d398281ec92e03c1d6f5f1bf93970debcfcc684.1565190104.git.korablev@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy > On 10 Aug 2019, at 00:02, Vladislav Shpilevoy = wrote: > Thanks for the patch! See 9 comments below. > On 07/08/2019 17:13, Nikita Pettik wrote: >> This procedure is processed in a several steps. Firstly, we add name >=20 > 1. Typo: 'a' is not used with plural. Shame on me, fixed. >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index 246654cb7..7d05ce11c 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -2867,8 +2859,7 @@ case OP_Savepoint: { >> * and this is a RELEASE command, then the = current transaction >> * is committed. >> */ >> - int isTransaction =3D pSavepoint->pNext =3D=3D = 0; >> - if (isTransaction && p1=3D=3DSAVEPOINT_RELEASE) = { >> + if (p1 =3D=3D SAVEPOINT_RELEASE) { >=20 > 2. First, the comment is outdated now (perhaps). > Second, I don't understand that code. Why release > of a savepoint >=20 > 1) affects fk? Because it is not rollback, it is just release. It > does not rollback anything, and only frees savepoint objects (freed, > when it was on malloc). >=20 > 2) leads to halt? Have no idea either. I just didn=E2=80=99t touch that code. Ok, since we started to refactor OP_Savepoint, let=E2=80=99s remove these strange routines as well.=20 >> if ((rc =3D sqlVdbeCheckFk(p, 1)) !=3D = 0) >> goto vdbe_return; >> sqlVdbeHalt(p); >> @@ -2876,19 +2867,8 @@ case OP_Savepoint: { >> goto abort_due_to_error; >> } else { >> if (p1=3D=3DSAVEPOINT_ROLLBACK) >> - = box_txn_rollback_to_savepoint(pSavepoint->tnt_savepoint); >> - } >> - >> - /* Regardless of whether this is a RELEASE or = ROLLBACK, destroy all >> - * savepoints nested inside of the savepoint = being operated on. >> - */ >> - while (psql_txn->pSavepoint !=3D pSavepoint) { >> - pTmp =3D psql_txn->pSavepoint; >> - psql_txn->pSavepoint =3D pTmp->pNext; >> - /* >> - * Since savepoints are stored in = region, we do not >> - * have to destroy them >> - */ >> + if = (box_txn_rollback_to_savepoint(sv) !=3D 0) >> + goto abort_due_to_error; >=20 > 3. Something is wrong with braces now. The top 'if' has two lines > in body, but has no braces. Honestly, the whole code block looks bad. > Please, consider these changes. They are below and on top of the = branch > in a separate commit. Seems good, applied. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 9165db6c3..7b0e450c5 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -2853,34 +2853,19 @@ case OP_Savepoint: { > if (sv =3D=3D NULL) { > diag_set(ClientError, ER_NO_SUCH_SAVEPOINT); > goto abort_due_to_error; > + } > + if (p1 =3D=3D SAVEPOINT_RELEASE) { > + if ((rc =3D sqlVdbeCheckFk(p, 1)) !=3D 0) > + goto vdbe_return; > + sqlVdbeHalt(p); > + if (p->is_aborted) > + goto abort_due_to_error; > + txn_savepoint_release(sv); > } else { > - > - /* Determine whether or not this is a = transaction savepoint. If so, > - * and this is a RELEASE command, then the = current transaction > - * is committed. > - */ > - if (p1 =3D=3D SAVEPOINT_RELEASE) { > - if ((rc =3D sqlVdbeCheckFk(p, 1)) !=3D = 0) > - goto vdbe_return; > - sqlVdbeHalt(p); > - if (p->is_aborted) > - goto abort_due_to_error; > - } else { > - if (p1=3D=3DSAVEPOINT_ROLLBACK) > - if = (box_txn_rollback_to_savepoint(sv) !=3D 0) > - goto abort_due_to_error; > - } > - > - /* If it is a RELEASE, then destroy the = savepoint being operated on > - * too. If it is a ROLLBACK TO, then set the = number of deferred > - * constraint violations present in the database = to the value stored > - * when the savepoint was created. > - */ > - if (p1=3D=3DSAVEPOINT_RELEASE) { > - txn_savepoint_release(sv); > - } else { > - txn->fk_deferred_count =3D = sv->fk_deferred_count; > - } > + assert(p1 =3D=3D SAVEPOINT_ROLLBACK); > + if (box_txn_rollback_to_savepoint(sv) !=3D 0) > + goto abort_due_to_error; > + txn->fk_deferred_count =3D = sv->fk_deferred_count; > } > } >=20 > = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D >=20 >> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h >> index 8f16202ba..06f258805 100644 >> --- a/src/box/sql/vdbe.h >> +++ b/src/box/sql/vdbe.h >> @@ -172,15 +172,6 @@ struct SubProgram { >> */ >> Vdbe *sqlVdbeCreate(Parse *); >>=20 >> -/** >> - * 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(); >=20 > 4. Please, drop sql_txn_begin too. OK: diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 7d05ce11c..cb7b585e2 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2918,7 +2918,11 @@ case OP_CheckViewReferences: { * Otherwise, raise an error with appropriate error message. */ case OP_TransactionBegin: { - if (sql_txn_begin() !=3D 0) + if (in_txn()) { + diag_set(ClientError, ER_ACTIVE_TRANSACTION); + goto abort_due_to_error; + } + if (txn_begin() =3D=3D NULL) goto abort_due_to_error; p->auto_commit =3D false ; break; @@ -2974,7 +2978,7 @@ case OP_TransactionRollback: { */ case OP_TTransaction: { if (!box_txn()) { - if (sql_txn_begin() !=3D 0) + if (txn_begin() =3D=3D NULL) goto abort_due_to_error; } else { p->anonymous_savepoint =3D sql_savepoint(p, NULL); diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 5582d9506..3050f75b7 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -453,8 +453,7 @@ u32 sqlVdbeSerialGet(const unsigned char *, u32, Mem = *); =20 int sqlVdbeExec(Vdbe *); int sqlVdbeList(Vdbe *); -int -sql_txn_begin(); + Savepoint * sql_savepoint(Vdbe *p, const char *zName); diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 3d256f86a..b77fffaec 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -1971,19 +1971,6 @@ sqlVdbeCheckFk(Vdbe * p, int deferred) return 0; } =20 -int -sql_txn_begin() -{ - if (in_txn()) { - diag_set(ClientError, ER_ACTIVE_TRANSACTION); - return -1; - } - struct txn *ptxn =3D txn_begin(false); - if (ptxn =3D=3D NULL) - return -1; - return 0; -} - Savepoint * sql_savepoint(MAYBE_UNUSED Vdbe *p, const char *zName) { >> diff --git a/src/box/txn.c b/src/box/txn.c >> index b57240846..451cc9eb1 100644 >> --- a/src/box/txn.c >> +++ b/src/box/txn.c >> @@ -779,10 +807,28 @@ = box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp) >> return -1; >> } >> txn_rollback_to_svp(txn, svp->stmt); >> + /* Discard from list all prior savepoints. */ >> + struct stailq discard; >> + stailq_cut_tail(&txn->savepoints, &svp->next, &discard); >> txn->fk_deferred_count =3D svp->fk_deferred_count; >> return 0; >> } >>=20 >> +void >> +txn_savepoint_release(struct txn_savepoint *svp) >> +{ >> + struct txn *txn =3D in_txn(); >> + assert(txn !=3D NULL); >=20 > 5. Please, add an assertion, that this savepoint is not released > yet. I.e. that its statement has non NULL space and row. Ok, but we should also consider savepoint containing empty statements list (like SAVEPOINT t1; RELEASE SAVEPOINT t1;): diff --git a/src/box/txn.c b/src/box/txn.c index 451cc9eb1..b112d3ad9 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -819,6 +819,11 @@ txn_savepoint_release(struct txn_savepoint *svp) { struct txn *txn =3D in_txn(); assert(txn !=3D NULL); + /* Make sure that savepoint hasn't been released yet. */ + assert(svp->in_sub_stmt =3D=3D 0 || + (stailq_entry(svp->stmt, struct txn_stmt, next)->space !=3D= NULL && + stailq_entry(svp->stmt, struct txn_stmt, next)->row !=3D = NULL)); >> + /* >> + * Discard current savepoint alongside with all >> + * created after it savepoints. Note that operation >> + * is currently available only for SQL. >> + */ >> + struct stailq discard; >> + stailq_cut_tail(&txn->savepoints, &svp->next, &discard); >> + stailq_cut_tail(&txn->savepoints, *txn->savepoints.last, = &discard); >=20 > 6. Sorry, I failed to understand what is happening here, and how > does it work. Why do you touch internal attributte of 'struct stailq', > and why the first call is not enough? stailq_cut_tail() cuts tail starting from the next entry, so it doesn=E2=80=99t remove savepoint itself from list. Yep, this code is obviously wrong (heh but our test suite doesn=E2=80=99t include test verifying it; surely it is my bad. But I had no doubts that such simple case is tested) since *last =3D=3D NULL, so list of savepoints is discarded entirely. Correct way is to find predecessor traversing the = list: diff --git a/src/box/txn.c b/src/box/txn.c index 8fc5f182e..aef655df2 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -820,15 +820,25 @@ txn_savepoint_release(struct txn_savepoint *svp) /* * Discard current savepoint alongside with all - * created after it savepoints. Note that operation - * is currently available only for SQL. + * created after it savepoints. To do so we should + * firstly find predecessor of the original node. Note + * that operation is currently available only for SQL. */ + struct stailq_entry *prev; + stailq_foreach(prev, &txn->savepoints) { + /* + * If savepoint to be removed is the first in the + * list, prev will be equal to NULL after + * iterations. So _cut_tail() is going to erase + * the whole list. + */ + if (stailq_next(prev) =3D=3D &svp->link) + break; + } struct stailq discard; - stailq_cut_tail(&txn->savepoints, &svp->link, &discard); - stailq_cut_tail(&txn->savepoints, *txn->savepoints.last, = &discard); + stailq_cut_tail(&txn->savepoints, prev, &discard); } =20 static void And test case I am talking about: diff --git a/test/sql/savepoints.test.lua b/test/sql/savepoints.test.lua index d96b04600..a4312c114 100644 --- a/test/sql/savepoints.test.lua +++ b/test/sql/savepoints.test.lua @@ -31,6 +31,18 @@ end; release_sv(); box.commit(); =20 +release_sv_2 =3D function() + box.begin() + box.execute('SAVEPOINT t1;') + box.execute('SAVEPOINT t2;') + box.execute('SAVEPOINT t3;') + box.execute('RELEASE SAVEPOINT t2;') + local _, err =3D box.execute('ROLLBACK TO t1;') + assert(err =3D=3D nil) +end; +release_sv_2(); +box.commit(); + >> diff --git a/src/box/txn.h b/src/box/txn.h >> index 87e880a6a..a14f3871a 100644 >> --- a/src/box/txn.h >> +++ b/src/box/txn.h >> @@ -128,15 +127,12 @@ struct txn_savepoint { >> * state violating any number of deferred FK constraints. >> */ >> uint32_t fk_deferred_count; >> + struct stailq_entry next; >=20 > 7. Why 'next'? It is current. 'next' is a part of > struct stailq_entry. Here you basically wrote 'next.next'. > We name queue members 'in_=E2=80=99. However, =E2=80=99next' prefix is used as well. Ofc I looked through source code before choosing a name :) struct stailq_entry next_in_log; in struct txv struct stailq_entry next; in struct vy_squash struct stailq_entry next; in struct txn_stmt struct stailq_entry next; in struct applier_tx_row Even in test/unit/stailq.c this member is called =E2=80=99next=E2=80=99. On second thought, I can say that =E2=80=99next=E2=80=99 really seems to = be a bit confusing. There=E2=80=99s also =E2=80=98link=E2=80=99 name: struct stailq_entry link; in struct memtx_gc_task struct stailq_entry link; in truct autoinc_id_entry IMHO =E2=80=98in_svps' sounds worse than just =E2=80=98link=E2=80=99. If = you don't mind I'll rename it to link. >> + char *name; >=20 > 8. I didn't like that name is stored by pointer - it consumes more > memory, and is slower. So I changed to name[1] with variable length > tail. But is it an advisory fix, you can skip it if you think it > complicates the code too much. See below and on the branch. >=20 > A mandatory remark - please, add a comment. And note here, that the > name is optional. If you stick to my solution, then mention, that an > omitted name is "". If you keep yours, then mention that name =3D=3D = NULL > means omission. Ok, I don=E2=80=99t mind your changes: I also tried both versions when was preparing patch, but failed to decide which is better. I don=E2=80=99t think that your version would get any = performance gain but I like that we don=E2=80=99t need check NULL pointer while iterating over the list and comparing names. Comment to the =E2=80=9Cname=E2=80=9D field: diff --git a/src/box/txn.h b/src/box/txn.h index 06092e3f9..3d378ce29 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -127,7 +127,16 @@ struct txn_savepoint { * state violating any number of deferred FK constraints. */ uint32_t fk_deferred_count; + /** Organize savepoints into linked list. */ struct stailq_entry link; + /** + * Optional name of savepoint. If savepoint lacks + * name (i.e. anonymous savepoint available only by + * reference to the object), name[0] =3D=3D ''. Otherwise, + * memory for name is reserved in the same memory chunk + * as struct txn_savepoint itself - name is placed + * right after structure (see txn_savepoint_new()). + */ char name[1]; }; >> @@ -455,6 +451,22 @@ txn_current_stmt(struct txn *txn) >> return stailq_entry(stmt, struct txn_stmt, next); >> } >>=20 >> +/** >> + * Allocate new savepoint object using region allocator. >> + * Savepoint is allowed to be anonymous (i.e. without >> + * name). >> + */ >> +struct txn_savepoint * >> +txn_savepoint_new(struct txn *txn, const char *name); >> + >> +/** Find savepoint by its name in savepoint list. */ >> +struct txn_savepoint * >> +txn_savepoint_by_name(struct txn *txn, const char *name); >> + >> +/** Remove given and all prior entries from savepoint list. */ >=20 > 9. I guess, all newer entries. Not prior (=3D older). Yep, fixed: diff --git a/src/box/txn.h b/src/box/txn.h index 50114a4a9..32d7f83f5 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -474,7 +474,7 @@ txn_savepoint_new(struct txn *txn, const char = *name); struct txn_savepoint * txn_savepoint_by_name(struct txn *txn, const char *name); =20 -/** Remove given and all prior entries from savepoint list. */ +/** Remove given and all newer entries from savepoint list. */ void txn_savepoint_release(struct txn_savepoint *svp); Whole patch: Author: Nikita Pettik Date: Fri Aug 2 19:40:55 2019 +0300 txn: merge struct sql_txn into struct txn =20 This procedure is processed in several steps. Firstly, we add name to struct txn_savepoint since we should be capable of operating on = named savepoints (which in turn is SQL feature). Still, anonymous (in the = sense of name absence) savepoints are also valid. Then, we add list (as implementation of stailq) of savepoints to struct txn: it allows us = to find savepoint by its name. Finally, we patch rollback to/release savepoint routines: for rollback tail of the list containing = savepoints is cut (but subject of rollback routine remains in the list); for release routine we cut tail including node being released. diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 246654cb7..0f4a160a7 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2823,13 +2823,9 @@ case OP_Count: { /* out2 */ case OP_Savepoint: { int p1; /* Value of P1 operand */ char *zName; /* Name of savepoint */ - Savepoint *pNew; - Savepoint *pSavepoint; - Savepoint *pTmp; struct txn *txn =3D in_txn(); - struct sql_txn *psql_txn =3D txn !=3D NULL ? txn->psql_txn : = NULL; =20 - if (psql_txn =3D=3D NULL) { + if (txn =3D=3D NULL) { assert(!box_txn()); diag_set(ClientError, ER_NO_TRANSACTION); goto abort_due_to_error; @@ -2840,69 +2836,31 @@ 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 =3D=3D NULL || box_txn()); + assert(stailq_empty(&txn->savepoints) || box_txn()); = assert(p1=3D=3DSAVEPOINT_BEGIN||p1=3D=3DSAVEPOINT_RELEASE||p1=3D=3DSAVEPOI= NT_ROLLBACK); =20 if (p1=3D=3DSAVEPOINT_BEGIN) { - /* Create a new savepoint structure. */ - pNew =3D sql_savepoint(p, zName); - /* Link the new savepoint into the database handle's = list. */ - pNew->pNext =3D psql_txn->pSavepoint; - psql_txn->pSavepoint =3D pNew; + /* + * Savepoint is available by its name so we don't + * care about object itself. + */ + (void) txn_savepoint_new(txn, zName); } else { /* Find the named savepoint. If there is no such = savepoint, then an * an error is returned to the user. */ - for( - pSavepoint =3D psql_txn->pSavepoint; - pSavepoint && sqlStrICmp(pSavepoint->zName, = zName); - pSavepoint =3D pSavepoint->pNext - ); - if (!pSavepoint) { + struct txn_savepoint *sv =3D txn_savepoint_by_name(txn, = zName); + if (sv =3D=3D NULL) { diag_set(ClientError, ER_NO_SUCH_SAVEPOINT); goto abort_due_to_error; + } + if (p1 =3D=3D SAVEPOINT_RELEASE) { + txn_savepoint_release(sv); } else { - - /* Determine whether or not this is a = transaction savepoint. If so, - * and this is a RELEASE command, then the = current transaction - * is committed. - */ - int isTransaction =3D pSavepoint->pNext =3D=3D = 0; - if (isTransaction && p1=3D=3DSAVEPOINT_RELEASE) = { - if ((rc =3D sqlVdbeCheckFk(p, 1)) !=3D = 0) - goto vdbe_return; - sqlVdbeHalt(p); - if (p->is_aborted) - goto abort_due_to_error; - } else { - if (p1=3D=3DSAVEPOINT_ROLLBACK) - = box_txn_rollback_to_savepoint(pSavepoint->tnt_savepoint); - } - - /* Regardless of whether this is a RELEASE or = ROLLBACK, destroy all - * savepoints nested inside of the savepoint = being operated on. - */ - while (psql_txn->pSavepoint !=3D pSavepoint) { - pTmp =3D psql_txn->pSavepoint; - psql_txn->pSavepoint =3D pTmp->pNext; - /* - * Since savepoints are stored in = region, we do not - * have to destroy them - */ - } - - /* If it is a RELEASE, then destroy the = savepoint being operated on - * too. If it is a ROLLBACK TO, then set the = number of deferred - * constraint violations present in the database = to the value stored - * when the savepoint was created. - */ - if (p1=3D=3DSAVEPOINT_RELEASE) { - assert(pSavepoint =3D=3D = psql_txn->pSavepoint); - psql_txn->pSavepoint =3D = pSavepoint->pNext; - } else { - txn->fk_deferred_count =3D - = pSavepoint->tnt_savepoint->fk_deferred_count; - } + assert(p1 =3D=3D SAVEPOINT_ROLLBACK); + if (box_txn_rollback_to_savepoint(sv) !=3D 0) + goto abort_due_to_error; + txn->fk_deferred_count =3D = sv->fk_deferred_count; } } =20 @@ -2940,7 +2898,11 @@ case OP_CheckViewReferences: { * Otherwise, raise an error with appropriate error message. */ case OP_TransactionBegin: { - if (sql_txn_begin() !=3D 0) + if (in_txn()) { + diag_set(ClientError, ER_ACTIVE_TRANSACTION); + goto abort_due_to_error; + } + if (txn_begin() =3D=3D NULL) goto abort_due_to_error; p->auto_commit =3D false ; break; @@ -2996,7 +2958,7 @@ case OP_TransactionRollback: { */ case OP_TTransaction: { if (!box_txn()) { - if (sql_txn_begin() !=3D 0) + if (txn_begin() =3D=3D NULL) goto abort_due_to_error; } else { p->anonymous_savepoint =3D sql_savepoint(p, NULL); @@ -4843,7 +4805,6 @@ case OP_FkCounter: { if (((p->sql_flags & SQL_DeferFKs) !=3D 0 || pOp->p1 !=3D 0) && !p->auto_commit) { struct txn *txn =3D in_txn(); - assert(txn !=3D NULL && txn->psql_txn !=3D NULL); txn->fk_deferred_count +=3D pOp->p2; } else { p->nFkConstraint +=3D pOp->p2; @@ -4867,7 +4828,6 @@ case OP_FkIfZero: { /* jump */ if (((p->sql_flags & SQL_DeferFKs) !=3D 0 || pOp->p1 !=3D 0) && !p->auto_commit) { struct txn *txn =3D in_txn(); - assert(txn !=3D NULL && txn->psql_txn !=3D NULL); if (txn->fk_deferred_count =3D=3D 0) goto jump_to_p2; } else { diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h index 8f16202ba..06f258805 100644 --- a/src/box/sql/vdbe.h +++ b/src/box/sql/vdbe.h @@ -172,15 +172,6 @@ struct SubProgram { */ Vdbe *sqlVdbeCreate(Parse *); =20 -/** - * 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 diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 5582d9506..3050f75b7 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -453,8 +453,7 @@ u32 sqlVdbeSerialGet(const unsigned char *, u32, Mem = *); =20 int sqlVdbeExec(Vdbe *); int sqlVdbeList(Vdbe *); -int -sql_txn_begin(); + Savepoint * sql_savepoint(Vdbe *p, const char *zName); diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index bbbb99c70..b77fffaec 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -76,39 +76,12 @@ sqlVdbeCreate(Parse * pParse) return p; } =20 -struct sql_txn * -sql_alloc_txn() -{ - struct sql_txn *txn =3D region_alloc_object(&fiber()->gc, - struct sql_txn); - if (txn =3D=3D NULL) { - diag_set(OutOfMemory, sizeof(struct sql_txn), "region", - "struct sql_txn"); - return NULL; - } - txn->pSavepoint =3D NULL; - return txn; -} - int sql_vdbe_prepare(struct Vdbe *vdbe) { assert(vdbe !=3D NULL); struct txn *txn =3D in_txn(); vdbe->auto_commit =3D txn =3D=3D NULL; - if (txn !=3D 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 =3D=3D NULL) { - txn->psql_txn =3D sql_alloc_txn(); - if (txn->psql_txn =3D=3D NULL) - return -1; - } - } return 0; } =20 @@ -1998,24 +1971,6 @@ sqlVdbeCheckFk(Vdbe * p, int deferred) return 0; } =20 -int -sql_txn_begin() -{ - if (in_txn()) { - diag_set(ClientError, ER_ACTIVE_TRANSACTION); - return -1; - } - struct txn *ptxn =3D txn_begin(false); - if (ptxn =3D=3D NULL) - return -1; - ptxn->psql_txn =3D sql_alloc_txn(); - if (ptxn->psql_txn =3D=3D NULL) { - box_txn_rollback(); - return -1; - } - return 0; -} - Savepoint * sql_savepoint(MAYBE_UNUSED Vdbe *p, const char *zName) { diff --git a/src/box/txn.c b/src/box/txn.c index b57240846..aef655df2 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -221,8 +221,8 @@ txn_begin() txn->signature =3D -1; txn->engine =3D NULL; txn->engine_tx =3D NULL; - txn->psql_txn =3D NULL; txn->fk_deferred_count =3D 0; + stailq_create(&txn->savepoints); txn->fiber =3D NULL; fiber_set_txn(fiber(), txn); /* fiber_on_yield is initialized by engine on demand */ @@ -734,28 +734,53 @@ box_txn_alloc(size_t size) alignof(union natural_align)); } =20 -box_txn_savepoint_t * -box_txn_savepoint() +struct txn_savepoint * +txn_savepoint_new(struct txn *txn, const char *name) { - struct txn *txn =3D in_txn(); - if (txn =3D=3D NULL) { - diag_set(ClientError, ER_NO_TRANSACTION); - return NULL; - } + assert(txn =3D=3D in_txn()); + size_t svp_sz =3D sizeof(struct txn_savepoint); + int name_len =3D name !=3D NULL ? strlen(name) : 0; + svp_sz +=3D name_len; struct txn_savepoint *svp =3D - (struct txn_savepoint *) = region_alloc_object(&txn->region, - struct = txn_savepoint); + (struct txn_savepoint *) region_alloc(&txn->region, = svp_sz); if (svp =3D=3D NULL) { - diag_set(OutOfMemory, sizeof(*svp), - "region", "struct txn_savepoint"); + diag_set(OutOfMemory, svp_sz, "region", "svp"); return NULL; } svp->stmt =3D stailq_last(&txn->stmts); svp->in_sub_stmt =3D txn->in_sub_stmt; svp->fk_deferred_count =3D txn->fk_deferred_count; + if (name !=3D NULL) + memcpy(svp->name, name, name_len + 1); + else + svp->name[0] =3D 0; + stailq_add_tail_entry(&txn->savepoints, svp, link); return svp; } =20 +struct txn_savepoint * +txn_savepoint_by_name(struct txn *txn, const char *name) +{ + assert(txn =3D=3D in_txn()); + struct txn_savepoint *sv; + stailq_foreach_entry(sv, &txn->savepoints, link) { + if (strcmp(sv->name, name) =3D=3D 0) + return sv; + } + return NULL; +} + +box_txn_savepoint_t * +box_txn_savepoint() +{ + struct txn *txn =3D in_txn(); + if (txn =3D=3D NULL) { + diag_set(ClientError, ER_NO_TRANSACTION); + return NULL; + } + return txn_savepoint_new(txn, NULL); +} + int box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp) { @@ -779,10 +804,43 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t = *svp) return -1; } txn_rollback_to_svp(txn, svp->stmt); + /* Discard from list all prior savepoints. */ + struct stailq discard; + stailq_cut_tail(&txn->savepoints, &svp->link, &discard); txn->fk_deferred_count =3D svp->fk_deferred_count; return 0; } =20 +void +txn_savepoint_release(struct txn_savepoint *svp) +{ + struct txn *txn =3D in_txn(); + assert(txn !=3D NULL); + /* Make sure that savepoint hasn't been released yet. */ + assert(svp->in_sub_stmt =3D=3D 0 || + (stailq_entry(svp->stmt, struct txn_stmt, next)->space !=3D= NULL && + stailq_entry(svp->stmt, struct txn_stmt, next)->row !=3D = NULL)); + /* + * Discard current savepoint alongside with all + * created after it savepoints. To do so we should + * firstly find predecessor of the original node. Note + * that operation is currently available only for SQL. + */ + struct stailq_entry *prev; + stailq_foreach(prev, &txn->savepoints) { + /* + * If savepoint to be removed is the first in the + * list, prev will be equal to NULL after + *iterations. So _cut_tail() is going to erase + * the whole list. + */ + if (stailq_next(prev) =3D=3D &svp->link) + break; + } + struct stailq discard; + stailq_cut_tail(&txn->savepoints, prev, &discard); +} + static void txn_on_stop(struct trigger *trigger, void *event) { diff --git a/src/box/txn.h b/src/box/txn.h index f795cb76f..3d378ce29 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -43,7 +43,6 @@ extern "C" { =20 /** box statistics */ extern struct rmean *rmean_box; -struct Savepoint; =20 struct engine; struct space; @@ -128,15 +127,21 @@ struct txn_savepoint { * state violating any number of deferred FK constraints. */ uint32_t fk_deferred_count; + /** Organize savepoints into linked list. */ + struct stailq_entry link; + /** + * Optional name of savepoint. If savepoint lacks + * name (i.e. anonymous savepoint available only by + * reference to the object), name[0] =3D=3D ''. Otherwise, + * memory for name is reserved in the same memory chunk + * as struct txn_savepoint itself - name is placed + * right after structure (see txn_savepoint_new()). + */ + char name[1]; }; =20 extern double too_long_threshold; =20 -struct sql_txn { - /** List of active SQL savepoints. */ - struct Savepoint *pSavepoint; -}; - /** * An element of list of autogenerated ids, being returned as SQL * response metadata. @@ -220,7 +225,7 @@ struct txn { * SQL specific property. */ uint32_t fk_deferred_count; - struct sql_txn *psql_txn; + struct stailq savepoints; }; =20 static inline bool @@ -466,6 +471,22 @@ txn_current_stmt(struct txn *txn) return stailq_entry(stmt, struct txn_stmt, next); } =20 +/** + * Allocate new savepoint object using region allocator. + * Savepoint is allowed to be anonymous (i.e. without + * name). + */ +struct txn_savepoint * +txn_savepoint_new(struct txn *txn, const char *name); + +/** Find savepoint by its name in savepoint list. */ +struct txn_savepoint * +txn_savepoint_by_name(struct txn *txn, const char *name); + +/** Remove given and all newer entries from savepoint list. */ +void +txn_savepoint_release(struct txn_savepoint *svp); + /** * FFI bindings: do not throw exceptions, do not accept extra * arguments diff --git a/test/sql/savepoints.result b/test/sql/savepoints.result index 78957173e..b5a0b7f46 100644 --- a/test/sql/savepoints.result +++ b/test/sql/savepoints.result @@ -58,6 +58,23 @@ release_sv(); box.commit(); --- ... +release_sv_2 =3D function() + box.begin() + box.execute('SAVEPOINT t1;') + box.execute('SAVEPOINT t2;') + box.execute('SAVEPOINT t3;') + box.execute('RELEASE SAVEPOINT t2;') + local _, err =3D box.execute('ROLLBACK TO t1;') + assert(err =3D=3D nil) +end; +--- +... +release_sv_2(); +--- +... +box.commit(); +--- +... release_sv_fail =3D function() box.begin() box.execute('SAVEPOINT t1;') diff --git a/test/sql/savepoints.test.lua b/test/sql/savepoints.test.lua index d96b04600..a4312c114 100644 --- a/test/sql/savepoints.test.lua +++ b/test/sql/savepoints.test.lua @@ -31,6 +31,18 @@ end; release_sv(); box.commit(); =20 +release_sv_2 =3D function() + box.begin() + box.execute('SAVEPOINT t1;') + box.execute('SAVEPOINT t2;') + box.execute('SAVEPOINT t3;') + box.execute('RELEASE SAVEPOINT t2;') + local _, err =3D box.execute('ROLLBACK TO t1;') + assert(err =3D=3D nil) +end; +release_sv_2(); +box.commit(); + release_sv_fail =3D function() box.begin() box.execute('SAVEPOINT t1;')