From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: "n.pettik" <korablev@tarantool.org>, tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn Date: Fri, 16 Aug 2019 00:03:59 +0200 [thread overview] Message-ID: <b080e29d-2878-d30b-be88-db8d614542d4@tarantool.org> (raw) In-Reply-To: <97A342BD-97BD-451F-A54B-A9CF18D352A3@tarantool.org> 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@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; > }
next prev parent reply other threads:[~2019-08-15 22:01 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-07 15:13 [tarantool-patches] [PATCH 0/3] Merge struct sql_txn into struct txn/savepoint Nikita Pettik 2019-08-07 15:13 ` [tarantool-patches] [PATCH 1/3] txn: move fk_deferred_count from psql_txn to txn Nikita Pettik 2019-08-09 20:59 ` [tarantool-patches] " Vladislav Shpilevoy 2019-08-15 11:03 ` n.pettik 2019-08-07 15:13 ` [tarantool-patches] [PATCH 2/3] txn: merge struct sql_txn into struct txn Nikita Pettik 2019-08-07 15:26 ` [tarantool-patches] " Konstantin Osipov 2019-08-09 21:02 ` Vladislav Shpilevoy 2019-08-12 21:55 ` Konstantin Osipov 2019-08-15 11:04 ` n.pettik 2019-08-15 22:03 ` Vladislav Shpilevoy [this message] 2019-08-16 18:52 ` n.pettik 2019-08-19 20:47 ` Vladislav Shpilevoy 2019-08-21 0:23 ` n.pettik 2019-08-21 20:45 ` Vladislav Shpilevoy 2019-08-07 15:13 ` [tarantool-patches] [PATCH 3/3] sql: use struct txn_savepoint as anonymous savepoint Nikita Pettik 2019-08-07 15:26 ` [tarantool-patches] Re: [PATCH 0/3] Merge struct sql_txn into struct txn/savepoint Konstantin Osipov 2019-08-22 11:56 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b080e29d-2878-d30b-be88-db8d614542d4@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox