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 770172846C for ; Fri, 16 Aug 2019 14:53:00 -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 jWnR-wn97QWB for ; Fri, 16 Aug 2019 14:53:00 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 0F16828465 for ; Fri, 16 Aug 2019 14:53:00 -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: Fri, 16 Aug 2019 21:52:57 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <7d398281ec92e03c1d6f5f1bf93970debcfcc684.1565190104.git.korablev@tarantool.org> <97A342BD-97BD-451F-A54B-A9CF18D352A3@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 >>>> 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. */ >=20 > 1. Again 'prior'. Should be 'newer', please. Fixed: diff --git a/src/box/txn.c b/src/box/txn.c index 875d8adc9..399436a43 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -804,7 +804,7 @@ 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. */ + /* Discard from list all newer savepoints. */ struct stailq discard; stailq_cut_tail(&txn->savepoints, &svp->link, &discard); txn->fk_deferred_count =3D svp->fk_deferred_count; >>>> + 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. >>=20 >> Ok, but we should also consider savepoint containing >> empty statements list (like SAVEPOINT t1; RELEASE SAVEPOINT t1;): >>=20 >> 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 || >=20 > 2. As far as I know, in_sub_stmt is always 0 between two statements. > (Unless they are in a trigger of another statement). This is because > txn_begin_stmt increments in_sub_stmt and txn_commit_stmt decrements. > This makes the assertion below always passing regardless of space and > row values, doesn't it? Not only for a first statement. Ok, then correct way to do this (I guess) is like in = box_txn_rollback_to_savepoint(). @@ -817,9 +817,10 @@ 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)); + struct txn_stmt *stmt =3D svp->stmt =3D=3D NULL ? NULL : + stailq_entry(svp->stmt, struct txn_stmt, = next); + assert(stmt =3D=3D NULL || (stmt->space !=3D NULL && stmt->row = !=3D NULL)); + (void) stmt; /* * Discard current savepoint alongside with all * created after it savepoints. To do so we should >> Author: Nikita Pettik >> Date: Fri Aug 2 19:40:55 2019 +0300 >>=20 >> 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. >>=20 >> 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 >> @@ -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); >=20 > 3. You still need to check for NULL in case of OOM. Indeed, fixed: diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 0f4a160a7..297a9b6c6 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2844,7 +2844,8 @@ case OP_Savepoint: { * Savepoint is available by its name so we don't * care about object itself. */ - (void) txn_savepoint_new(txn, zName); + if (txn_savepoint_new(txn, zName) =3D=3D NULL) + goto abort_due_to_error; } else { /* Find the named savepoint. If there is no such = savepoint, then an * an error is returned to the user. >> - /* 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 > 4. Exactly the same assignment is in box_txn_rollback_to_savepoint. It > is not needed here anymore. Removed: @@ -2860,7 +2861,6 @@ case OP_Savepoint: { 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; } } >> 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 >> @@ -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) >=20 > 5. Looks like this function allows duplicate names. Is it > done on purpose? It=E2=80=99s an interesting question. I thought that it is OK. For = instance, PostgreSQL allows multiple savepoints with the same name: https://www.postgresql.org/docs/8.1/sql-savepoint.html =E2=80=98=E2=80=99=E2=80=99 SQL requires a savepoint to be destroyed automatically when another savepoint with the same name is established. In PostgreSQL, the old savepoint is kept, though only the more recent one will be used when rolling back or releasing. (Releasing the newer savepoint will cause the older one to again become accessible to ROLLBACK TO SAVEPOINT and RELEASE SAVEPOINT.) Otherwise, SAVEPOINT is fully SQL conforming. =E2=80=98=E2=80=99' Same for MS SQL: = https://docs.microsoft.com/en-us/sql/t-sql/language-elements/save-transact= ion-transact-sql?view=3Dsql-server-2017 =E2=80=98=E2=80=99=E2=80=99 Duplicate savepoint names are allowed in a transaction, but a ROLLBACK TRANSACTION statement that specifies the savepoint name will only roll the transaction back to the most recent SAVE TRANSACTION using that name. =E2=80=98=E2=80=99' Meanwhile MySQL and DB2 process name duplicates according to ANSI: 17.5 1) Let S be the . 2) If S identifies an existing savepoint established within the current savepoint level, then that savepoint is destroyed. MySQL: https://dev.mysql.com/doc/refman/8.0/en/savepoint.html =E2=80=98=E2=80=99=E2=80=99 The SAVEPOINT statement sets a named transaction savepoint with a name of identifier. If the current transaction has a savepoint with the same name, the old savepoint is deleted and a new one is set. =E2=80=98'' In DB2 things are a bit complicated with =E2=80=98UNIQUE=E2=80=99 = specifier, but if it is omitted, savepoint with the same name is erased: = https://www.ibm.com/support/knowledgecenter/en/SSEPEK_10.0.0/sqlref/src/tp= c/db2z_sql_savepoint.html =E2=80=98=E2=80=99=E2=80=99 If svpt-name identifies a savepoint that already exists within the unit of recovery and the savepoint was not created with the UNIQUE option, the existing savepoint is destroyed and a new savepoint is created. =E2=80=98=E2=80=99' But our current behaviour is even different from PosgreSQL and MSSQL: they use most recent savepoint (i.e. they add savepoints to the head of list), meanwhile we use oldest (i.e. we add savepoint to the tail of list). Hence, release of savepoint releases all savepoints starting from the very first created with given name; same for rollback. Currently, our doc says: =E2=80=98'' If a savepoint with the same name already exists, it is released before the new savepoint is set. =E2=80=98=E2=80=99' Which is obviously wrong (both on master and new branches): we don=E2=80=99t release savepoints: tarantool> box.begin() > box.execute('SAVEPOINT t1;') > _, err =3D box.execute('SAVEPOINT t1;') > print(err) > _, err =3D box.execute('RELEASE SAVEPOINT t1;') > print(err) > _, err =3D box.execute('RELEASE SAVEPOINT t1;') > print(err) > _, err =3D box.execute('RELEASE SAVEPOINT t1;') > print(err) > box.commit(); nil nil nil Can not rollback to savepoint: the savepoint does not exist --- =E2=80=A6 So, on the master branch duplicates are allowed. On the new branch: tarantool> box.begin() > box.execute('SAVEPOINT t1;') > _, err =3D box.execute('SAVEPOINT t1;') > print(err) > _, err =3D box.execute('RELEASE SAVEPOINT t1;') > print(err) > _, err =3D box.execute('RELEASE SAVEPOINT t1;') > print(err) > _, err =3D box.execute('RELEASE SAVEPOINT t1;') > print(err) > box.commit(); nil nil Can not rollback to savepoint: the savepoint does not exist Can not rollback to savepoint: the savepoint does not exist =E2=80=94 =E2=80=A6 Let=E2=80=99s fix that and always release old savepoint as doc says and ANSI declares. To do so, it=E2=80=99s better to use double linked list - it allows to remove entry without additional iterations over the list. Diff to the current patch: diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 14d6a2317..17ab19078 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2836,7 +2836,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(stailq_empty(&txn->savepoints) || box_txn()); + assert(rlist_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) { diff --git a/src/box/txn.c b/src/box/txn.c index 3cfe4348f..e102c0501 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -222,7 +222,7 @@ txn_begin() txn->engine =3D NULL; txn->engine_tx =3D NULL; txn->fk_deferred_count =3D 0; - stailq_create(&txn->savepoints); + rlist_create(&txn->savepoints); txn->fiber =3D NULL; fiber_set_txn(fiber(), txn); /* fiber_on_yield is initialized by engine on demand */ @@ -754,7 +754,7 @@ txn_savepoint_new(struct txn *txn, const char *name) memcpy(svp->name, name, name_len + 1); else svp->name[0] =3D 0; - stailq_add_tail_entry(&txn->savepoints, svp, link); + rlist_add_entry(&txn->savepoints, svp, link); return svp; } =20 @@ -763,7 +763,7 @@ 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) { + rlist_foreach_entry(sv, &txn->savepoints, link) { if (strcmp(sv->name, name) =3D=3D 0) return sv; } @@ -805,8 +805,8 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t = *svp) } txn_rollback_to_svp(txn, svp->stmt); /* Discard from list all newer savepoints. */ - struct stailq discard; - stailq_cut_tail(&txn->savepoints, &svp->link, &discard); + struct rlist discard =3D { }; + rlist_cut_before(&discard, &txn->savepoints, &svp->link); txn->fk_deferred_count =3D svp->fk_deferred_count; return 0; } @@ -823,23 +823,10 @@ txn_savepoint_release(struct txn_savepoint *svp) (void) stmt; /* * 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. + * created after it savepoints. */ - 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); + struct rlist discard =3D { }; + rlist_cut_before(&discard, &txn->savepoints, = rlist_next(&svp->link)); } =20 static void diff --git a/src/box/txn.h b/src/box/txn.h index 3d378ce29..da12feebf 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -128,7 +128,7 @@ struct txn_savepoint { */ uint32_t fk_deferred_count; /** Organize savepoints into linked list. */ - struct stailq_entry link; + struct rlist link; /** * Optional name of savepoint. If savepoint lacks * name (i.e. anonymous savepoint available only by @@ -225,7 +225,8 @@ struct txn { * SQL specific property. */ uint32_t fk_deferred_count; - struct stailq savepoints; + /** List of savepoints to find savepoint by name. */ + struct rlist savepoints; }; =20 static inline bool New follow-up patch: txn: erase old savepoint in case of name collision =20 Name duplicates are allowed for savepoints (both in our SQL implementation and in ANSI specification). ANSI SQL states that = previous savepoint should be deleted. What is more, our doc confirms this = fact and says that "...it is released before the new savepoint is set." Unfortunately, it's not true - currently old savepoint remains in = the list. For instance: =20 SAVEPOINT t; SAVEPOINT t; RELEASE SAVEPOINT t; RELEASE SAVEPOINT t; -- no error is raised =20 Let's fix this and remove old savepoint from the list. diff --git a/src/box/txn.c b/src/box/txn.c index e102c0501..2777608e1 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -750,10 +750,20 @@ txn_savepoint_new(struct txn *txn, const char = *name) 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) + if (name !=3D NULL) { + /* + * If savepoint with given name already exists, + * erase it from the list. This has to be done + * in accordance with ANSI SQL compliance. + */ + struct txn_savepoint *old_svp =3D + txn_savepoint_by_name(txn, name); + if (old_svp !=3D NULL) + rlist_del(&old_svp->link); memcpy(svp->name, name, name_len + 1); - else + } else { svp->name[0] =3D 0; + } rlist_add_entry(&txn->savepoints, svp, link); return svp; } diff --git a/test/sql/savepoints.result b/test/sql/savepoints.result index b5a0b7f46..e48db302a 100644 --- a/test/sql/savepoints.result +++ b/test/sql/savepoints.result @@ -95,3 +95,27 @@ release_sv_fail(); box.commit(); --- ... +-- Make sure that if the current transaction has a savepoint +-- with the same name, the old savepoint is deleted and +-- a new one is set. Note that no error should be raised. +-- +collision_sv_2 =3D function() + box.begin() + box.execute('SAVEPOINT t1;') + box.execute('SAVEPOINT t2;') + local _,err =3D box.execute('SAVEPOINT t1;') + assert(err =3D=3D nil) + box.execute('RELEASE SAVEPOINT t1;') + local _,err =3D box.execute('RELEASE SAVEPOINT t1;') + assert(err ~=3D nil) + local _, err =3D box.execute('ROLLBACK TO t2;') + assert(err =3D=3D nil) +end; +--- +... +collision_sv_2(); +--- +... +box.commit(); +--- +... diff --git a/test/sql/savepoints.test.lua b/test/sql/savepoints.test.lua index a4312c114..99622a4db 100644 --- a/test/sql/savepoints.test.lua +++ b/test/sql/savepoints.test.lua @@ -56,3 +56,22 @@ release_sv_fail =3D function() end; release_sv_fail(); box.commit(); + +-- Make sure that if the current transaction has a savepoint +-- with the same name, the old savepoint is deleted and +-- a new one is set. Note that no error should be raised. +-- +collision_sv_2 =3D function() + box.begin() + box.execute('SAVEPOINT t1;') + box.execute('SAVEPOINT t2;') + local _,err =3D box.execute('SAVEPOINT t1;') + assert(err =3D=3D nil) + box.execute('RELEASE SAVEPOINT t1;') + local _,err =3D box.execute('RELEASE SAVEPOINT t1;') + assert(err ~=3D nil) + local _, err =3D box.execute('ROLLBACK TO t2;') + assert(err =3D=3D nil) +end; +collision_sv_2(); +box.commit();