[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Aug 10 00:02:44 MSK 2019
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
1. Typo: 'a' is not used with plural.
> 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.
> ---
> src/box/sql/vdbe.c | 52 +++++++++++---------------------------
> src/box/sql/vdbe.h | 9 -------
> src/box/sql/vdbeaux.c | 32 -----------------------
> src/box/txn.c | 70 ++++++++++++++++++++++++++++++++++++++++++---------
> src/box/txn.h | 26 +++++++++++++------
> 5 files changed, 91 insertions(+), 98 deletions(-)
>
> 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 = pSavepoint->pNext == 0;
> - if (isTransaction && p1==SAVEPOINT_RELEASE) {
> + if (p1 == SAVEPOINT_RELEASE) {
2. First, the comment is outdated now (perhaps).
Second, I don't understand that code. Why release
of a savepoint
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).
2) leads to halt?
> if ((rc = sqlVdbeCheckFk(p, 1)) != 0)
> goto vdbe_return;
> sqlVdbeHalt(p);
> @@ -2876,19 +2867,8 @@ case OP_Savepoint: {
> 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 (box_txn_rollback_to_savepoint(sv) != 0)
> + goto abort_due_to_error;
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.
=================================================================================
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 == NULL) {
diag_set(ClientError, ER_NO_SUCH_SAVEPOINT);
goto abort_due_to_error;
+ }
+ if (p1 == SAVEPOINT_RELEASE) {
+ if ((rc = sqlVdbeCheckFk(p, 1)) != 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 == 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)
- if (box_txn_rollback_to_savepoint(sv) != 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==SAVEPOINT_RELEASE) {
- txn_savepoint_release(sv);
- } else {
- txn->fk_deferred_count = sv->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;
}
}
=================================================================================
> 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 *);
>
> -/**
> - * 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();
4. Please, drop sql_txn_begin too.
> 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 = 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.
> + /*
> + * 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);
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?
> +}
> +
> static void
> txn_on_stop(struct trigger *trigger, void *event)
> {
> 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>'.
> + char *name;
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.
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 == NULL
means omission.
=================================================================================
diff --git a/src/box/txn.c b/src/box/txn.c
index 451cc9eb1..1b9713348 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -739,7 +739,8 @@ txn_savepoint_new(struct txn *txn, const char *name)
{
assert(txn == in_txn());
size_t svp_sz = sizeof(struct txn_savepoint);
- svp_sz += name != NULL ? strlen(name) + 1 : 0;
+ int name_len = name != NULL ? strlen(name) : 0;
+ svp_sz += name_len;
struct txn_savepoint *svp =
(struct txn_savepoint *) region_alloc(&txn->region, svp_sz);
if (svp == NULL) {
@@ -749,12 +750,10 @@ 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) {
- svp->name = (char *) svp + sizeof(struct txn_savepoint);
- memcpy(svp->name, name, strlen(name) + 1);
- } else {
- svp->name = NULL;
- }
+ if (name != NULL)
+ memcpy(svp->name, name, name_len + 1);
+ else
+ svp->name[0] = 0;
stailq_add_tail_entry(&txn->savepoints, svp, next);
return svp;
}
@@ -763,10 +762,8 @@ struct txn_savepoint *
txn_savepoint_by_name(struct txn *txn, const char *name)
{
assert(txn == in_txn());
- struct txn_savepoint *sv = NULL;
+ struct txn_savepoint *sv;
stailq_foreach_entry(sv, &txn->savepoints, next) {
- if (sv->name == NULL)
- continue;
if (strcmp(sv->name, name) == 0)
return sv;
}
diff --git a/src/box/txn.h b/src/box/txn.h
index a14f3871a..4f931f68f 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -128,7 +128,7 @@ struct txn_savepoint {
*/
uint32_t fk_deferred_count;
struct stailq_entry next;
- char *name;
+ char name[1];
};
extern double too_long_threshold;
=================================================================================
> @@ -455,6 +451,22 @@ txn_current_stmt(struct txn *txn)
> return stailq_entry(stmt, struct txn_stmt, next);
> }
>
> +/**
> + * 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. */
9. I guess, all newer entries. Not prior (= older).
More information about the Tarantool-patches
mailing list