[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