From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D31292363C for ; Thu, 15 Aug 2019 18:01:15 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Eaed-9Zgn6Lz for ; Thu, 15 Aug 2019 18:01:15 -0400 (EDT) Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 62C0223484 for ; Thu, 15 Aug 2019 18:01:15 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn References: <7d398281ec92e03c1d6f5f1bf93970debcfcc684.1565190104.git.korablev@tarantool.org> <97A342BD-97BD-451F-A54B-A9CF18D352A3@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 16 Aug 2019 00:03:59 +0200 MIME-Version: 1.0 In-Reply-To: <97A342BD-97BD-451F-A54B-A9CF18D352A3@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: "n.pettik" , tarantool-patches@freelists.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_’. > > 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 > 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; > }