[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Aug 16 01:03:59 MSK 2019
Hi! Thanks for the fixes!
See 5 comments below.
>>> 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.
>>> + 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.
>>> 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;
>>
>> 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_<queue_name>’.
>
> However, ’next' 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 ’next’.
>
> On second thought, I can say that ’next’ really seems to be
> a bit confusing. There’s also ‘link’ name:
>
> struct stailq_entry link; in struct memtx_gc_task
> struct stailq_entry link; in truct autoinc_id_entry
>
> IMHO ‘in_svps' sounds worse than just ‘link’. If you don't
> mind I'll rename it to link.
Thanks for the investigation. Ok, if 'next' is a really
household name here, then you can keep it. Up to you.
> 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.
> } else {
> /* Find the named savepoint. If there is no such savepoint, then an
> * an error is returned to the user.
> */
> - for(
> - pSavepoint = psql_txn->pSavepoint;
> - pSavepoint && sqlStrICmp(pSavepoint->zName, zName);
> - pSavepoint = pSavepoint->pNext
> - );
> - if (!pSavepoint) {
> + struct txn_savepoint *sv = txn_savepoint_by_name(txn, zName);
> + if (sv == NULL) {
> diag_set(ClientError, ER_NO_SUCH_SAVEPOINT);
> goto abort_due_to_error;
> + }
> + if (p1 == 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 = pSavepoint->pNext == 0;
> - if (isTransaction && p1==SAVEPOINT_RELEASE) {
> - if ((rc = sqlVdbeCheckFk(p, 1)) != 0)
> - goto vdbe_return;
> - sqlVdbeHalt(p);
> - if (p->is_aborted)
> - goto abort_due_to_error;
> - } else {
> - if (p1==SAVEPOINT_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 != pSavepoint) {
> - pTmp = psql_txn->pSavepoint;
> - psql_txn->pSavepoint = 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==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.
> }
> }
>
> 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?
> {
> - struct txn *txn = in_txn();
> - if (txn == NULL) {
> - diag_set(ClientError, ER_NO_TRANSACTION);
> - return NULL;
> - }
> + assert(txn == in_txn());
> + size_t svp_sz = sizeof(struct txn_savepoint);
> + int name_len = name != NULL ? strlen(name) : 0;
> + svp_sz += name_len;
> struct txn_savepoint *svp =
> - (struct txn_savepoint *) region_alloc_object(&txn->region,
> - struct txn_savepoint);
> + (struct txn_savepoint *) region_alloc(&txn->region, svp_sz);
> if (svp == NULL) {
> - diag_set(OutOfMemory, sizeof(*svp),
> - "region", "struct txn_savepoint");
> + diag_set(OutOfMemory, svp_sz, "region", "svp");
> return NULL;
> }
More information about the Tarantool-patches
mailing list