[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn
n.pettik
korablev at tarantool.org
Fri Aug 16 21:52:57 MSK 2019
>>>> 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. */
>
> 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 = svp->fk_deferred_count;
>>>> + struct stailq discard;
>>>> + stailq_cut_tail(&txn->savepoints, &svp->next, &discard);
>>>> txn->fk_deferred_count = svp->fk_deferred_count;
>>>> return 0;
>>>> }
>>>>
>>>> +void
>>>> +txn_savepoint_release(struct txn_savepoint *svp)
>>>> +{
>>>> + struct txn *txn = in_txn();
>>>> + assert(txn != NULL);
>>>
>>> 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 = in_txn();
>> assert(txn != NULL);
>> + /* Make sure that savepoint hasn't been released yet. */
>> + assert(svp->in_sub_stmt == 0 ||
>
> 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 = in_txn();
assert(txn != NULL);
/* Make sure that savepoint hasn't been released yet. */
- assert(svp->in_sub_stmt == 0 ||
- (stailq_entry(svp->stmt, struct txn_stmt, next)->space != NULL &&
- stailq_entry(svp->stmt, struct txn_stmt, next)->row != NULL));
+ struct txn_stmt *stmt = svp->stmt == NULL ? NULL :
+ stailq_entry(svp->stmt, struct txn_stmt, next);
+ assert(stmt == NULL || (stmt->space != NULL && stmt->row != NULL));
+ (void) stmt;
/*
* Discard current savepoint alongside with all
* created after it savepoints. To do so we should
>> Author: Nikita Pettik <korablev at tarantool.org>
>> Date: Fri Aug 2 19:40:55 2019 +0300
>>
>> txn: merge struct sql_txn into struct txn
>>
>> 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
>> @@ -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 == NULL || box_txn());
>> + assert(stailq_empty(&txn->savepoints) || box_txn());
>> assert(p1==SAVEPOINT_BEGIN||p1==SAVEPOINT_RELEASE||p1==SAVEPOINT_ROLLBACK);
>>
>> if (p1==SAVEPOINT_BEGIN) {
>> - /* Create a new savepoint structure. */
>> - pNew = sql_savepoint(p, zName);
>> - /* Link the new savepoint into the database handle's list. */
>> - pNew->pNext = psql_txn->pSavepoint;
>> - psql_txn->pSavepoint = pNew;
>> + /*
>> + * Savepoint is available by its name so we don't
>> + * care about object itself.
>> + */
>> + (void) txn_savepoint_new(txn, zName);
>
> 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) == 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==SAVEPOINT_RELEASE) {
>> - assert(pSavepoint == psql_txn->pSavepoint);
>> - psql_txn->pSavepoint = pSavepoint->pNext;
>> - } else {
>> - txn->fk_deferred_count =
>> - pSavepoint->tnt_savepoint->fk_deferred_count;
>> - }
>> + assert(p1 == SAVEPOINT_ROLLBACK);
>> + if (box_txn_rollback_to_savepoint(sv) != 0)
>> + goto abort_due_to_error;
>> + txn->fk_deferred_count = sv->fk_deferred_count;
>
> 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 == SAVEPOINT_ROLLBACK);
if (box_txn_rollback_to_savepoint(sv) != 0)
goto abort_due_to_error;
- txn->fk_deferred_count = 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));
>> }
>>
>> -box_txn_savepoint_t *
>> -box_txn_savepoint()
>> +struct txn_savepoint *
>> +txn_savepoint_new(struct txn *txn, const char *name)
>
> 5. Looks like this function allows duplicate names. Is it
> done on purpose?
It’s 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
‘’’
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.
‘’'
Same for MS SQL:
https://docs.microsoft.com/en-us/sql/t-sql/language-elements/save-transaction-transact-sql?view=sql-server-2017
‘’’
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.
‘’'
Meanwhile MySQL and DB2 process name duplicates
according to ANSI:
17.5 <savepoint statement>
1) Let S be the <savepoint name>.
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
‘’’
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.
‘''
In DB2 things are a bit complicated with ‘UNIQUE’ 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/tpc/db2z_sql_savepoint.html
‘’’
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.
‘’'
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:
‘''
If a savepoint with the same name already exists, it is released
before the new savepoint is set.
‘’'
Which is obviously wrong (both on master and new branches):
we don’t release savepoints:
tarantool> box.begin()
> box.execute('SAVEPOINT t1;')
> _, err = box.execute('SAVEPOINT t1;')
> print(err)
> _, err = box.execute('RELEASE SAVEPOINT t1;')
> print(err)
> _, err = box.execute('RELEASE SAVEPOINT t1;')
> print(err)
> _, err = box.execute('RELEASE SAVEPOINT t1;')
> print(err)
> box.commit();
nil
nil
nil
Can not rollback to savepoint: the savepoint does not exist
---
…
So, on the master branch duplicates are allowed. On the new branch:
tarantool> box.begin()
> box.execute('SAVEPOINT t1;')
> _, err = box.execute('SAVEPOINT t1;')
> print(err)
> _, err = box.execute('RELEASE SAVEPOINT t1;')
> print(err)
> _, err = box.execute('RELEASE SAVEPOINT t1;')
> print(err)
> _, err = 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
—
…
Let’s fix that and always release old savepoint as doc says
and ANSI declares. To do so, it’s 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==SAVEPOINT_BEGIN||p1==SAVEPOINT_RELEASE||p1==SAVEPOINT_ROLLBACK);
if (p1==SAVEPOINT_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 = NULL;
txn->engine_tx = NULL;
txn->fk_deferred_count = 0;
- stailq_create(&txn->savepoints);
+ rlist_create(&txn->savepoints);
txn->fiber = 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] = 0;
- stailq_add_tail_entry(&txn->savepoints, svp, link);
+ rlist_add_entry(&txn->savepoints, svp, link);
return svp;
}
@@ -763,7 +763,7 @@ txn_savepoint_by_name(struct txn *txn, const char *name)
{
assert(txn == 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) == 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 = { };
+ rlist_cut_before(&discard, &txn->savepoints, &svp->link);
txn->fk_deferred_count = 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) == &svp->link)
- break;
- }
- struct stailq discard;
- stailq_cut_tail(&txn->savepoints, prev, &discard);
+ struct rlist discard = { };
+ rlist_cut_before(&discard, &txn->savepoints, rlist_next(&svp->link));
}
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;
};
static inline bool
New follow-up patch:
txn: erase old savepoint in case of name collision
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:
SAVEPOINT t;
SAVEPOINT t;
RELEASE SAVEPOINT t;
RELEASE SAVEPOINT t; -- no error is raised
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 = stailq_last(&txn->stmts);
svp->in_sub_stmt = txn->in_sub_stmt;
svp->fk_deferred_count = txn->fk_deferred_count;
- if (name != NULL)
+ if (name != 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 =
+ txn_savepoint_by_name(txn, name);
+ if (old_svp != NULL)
+ rlist_del(&old_svp->link);
memcpy(svp->name, name, name_len + 1);
- else
+ } else {
svp->name[0] = 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 = function()
+ box.begin()
+ box.execute('SAVEPOINT t1;')
+ box.execute('SAVEPOINT t2;')
+ local _,err = box.execute('SAVEPOINT t1;')
+ assert(err == nil)
+ box.execute('RELEASE SAVEPOINT t1;')
+ local _,err = box.execute('RELEASE SAVEPOINT t1;')
+ assert(err ~= nil)
+ local _, err = box.execute('ROLLBACK TO t2;')
+ assert(err == 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 = 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 = function()
+ box.begin()
+ box.execute('SAVEPOINT t1;')
+ box.execute('SAVEPOINT t2;')
+ local _,err = box.execute('SAVEPOINT t1;')
+ assert(err == nil)
+ box.execute('RELEASE SAVEPOINT t1;')
+ local _,err = box.execute('RELEASE SAVEPOINT t1;')
+ assert(err ~= nil)
+ local _, err = box.execute('ROLLBACK TO t2;')
+ assert(err == nil)
+end;
+collision_sv_2();
+box.commit();
More information about the Tarantool-patches
mailing list