From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id DE4394765E0 for ; Tue, 29 Dec 2020 09:59:06 +0300 (MSK) Date: Tue, 29 Dec 2020 09:59:02 +0300 From: Mergen Imeev Message-ID: <20201229065902.GA620640@tarantool.org> References: <59789e72543d71f8b66dff8f02866af52508aa8b.1609205150.git.alexander.turenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <59789e72543d71f8b66dff8f02866af52508aa8b.1609205150.git.alexander.turenko@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] box: do not reset region on select List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy , Mergen Imeev Hi! I have no objections. On Tue, Dec 29, 2020 at 04:46:35AM +0300, Alexander Turenko wrote: > From: Mergen Imeev > > A read only transaction should not invalidate the fiber region. > > The problem was found in context of calling a Lua function from SQL on > tarantool 2.* (#5427). > > Another problem (#5623) was fixed by this commit on tarantool 2.*. Since > 2.2.0-469-g707e58a3f ('box: rework box_lua_{call, eval} to use input > port') arguments for a C stored procedure is allocated on a fiber > region. The region is truncated at the end of the call. When the > procedure performs a read only transaction, fiber_gc() may be called > and so region_truncate() will fail on assertion (on Debug build). > > Despite that all found cases are specific for tarantool 2.*, it is > possible to be hit by the problem on 1.10 as well. It is enough to > allocate something on a region (explicitly or implicitly) and perform a > read only transaction. > > So it looks worthful to backport the fix to 1.10 as well. > > (cherry picked from commit d0d668fa014763adefb71bfeddab1ae5a5350fce) > > The original commit message: > > | sql: do not reset region on select > | > | Prior to this patch, region on fiber was reset during select(), get(), > | count(), max(), or min(). This would result in an error if one of these > | operations was used in a user-defined function in SQL. After this patch, > | these functions truncate region instead of resetting it. > | > | Closes #5427 > --- > > Hi! > > Mergen, Vlad, Alexander, Kirill, > > I want to fix the problem on 1.10 as well, prior to 1.10.9 release. Are > there any objections? > > The patch is cherry picked without changes, except removed src/box/sql > files, removed test (it is SQL specific) and rewritten commit message. > > https://github.com/tarantool/tarantool/issues/5427 > https://github.com/tarantool/tarantool/issues/5623 > https://github.com/tarantool/tarantool/tree/Totktonada/gh-5427-ro-txn-dont-invalidate-fiber-region-1.10 > > Fails on GitHub Actions runners on > app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua are expected (and we > work on them). > > @ChangeLog (somewhere in 'Core'): Don't invalidate the fiber region > (the box region) on a read only transaction (gh-5427, gh-5623). > > src/box/box.cc | 5 +++-- > src/box/index.cc | 25 +++++++++++++++---------- > src/box/txn.h | 23 ++++++++++++++++++++--- > 3 files changed, 38 insertions(+), 15 deletions(-) > > diff --git a/src/box/box.cc b/src/box/box.cc > index 58011d44c..0f6761cf6 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1070,7 +1070,8 @@ box_select(uint32_t space_id, uint32_t index_id, > }); > > struct txn *txn; > - if (txn_begin_ro_stmt(space, &txn) != 0) > + struct txn_ro_savepoint svp; > + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) > return -1; > > struct iterator *it = index_create_iterator(index, type, > @@ -1104,7 +1105,7 @@ box_select(uint32_t space_id, uint32_t index_id, > txn_rollback_stmt(); > return -1; > } > - txn_commit_ro_stmt(txn); > + txn_commit_ro_stmt(txn, &svp); > return 0; > } > > diff --git a/src/box/index.cc b/src/box/index.cc > index 9c4acec1d..bb218061f 100644 > --- a/src/box/index.cc > +++ b/src/box/index.cc > @@ -246,13 +246,14 @@ box_index_get(uint32_t space_id, uint32_t index_id, const char *key, > return -1; > /* Start transaction in the engine. */ > struct txn *txn; > - if (txn_begin_ro_stmt(space, &txn) != 0) > + struct txn_ro_savepoint svp; > + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) > return -1; > if (index_get(index, key, part_count, result) != 0) { > txn_rollback_stmt(); > return -1; > } > - txn_commit_ro_stmt(txn); > + txn_commit_ro_stmt(txn, &svp); > /* Count statistics. */ > rmean_collect(rmean_box, IPROTO_SELECT, 1); > if (*result != NULL) > @@ -280,13 +281,14 @@ box_index_min(uint32_t space_id, uint32_t index_id, const char *key, > return -1; > /* Start transaction in the engine. */ > struct txn *txn; > - if (txn_begin_ro_stmt(space, &txn) != 0) > + struct txn_ro_savepoint svp; > + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) > return -1; > if (index_min(index, key, part_count, result) != 0) { > txn_rollback_stmt(); > return -1; > } > - txn_commit_ro_stmt(txn); > + txn_commit_ro_stmt(txn, &svp); > if (*result != NULL) > tuple_bless(*result); > return 0; > @@ -312,13 +314,14 @@ box_index_max(uint32_t space_id, uint32_t index_id, const char *key, > return -1; > /* Start transaction in the engine. */ > struct txn *txn; > - if (txn_begin_ro_stmt(space, &txn) != 0) > + struct txn_ro_savepoint svp; > + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) > return -1; > if (index_max(index, key, part_count, result) != 0) { > txn_rollback_stmt(); > return -1; > } > - txn_commit_ro_stmt(txn); > + txn_commit_ro_stmt(txn, &svp); > if (*result != NULL) > tuple_bless(*result); > return 0; > @@ -345,14 +348,15 @@ box_index_count(uint32_t space_id, uint32_t index_id, int type, > return -1; > /* Start transaction in the engine. */ > struct txn *txn; > - if (txn_begin_ro_stmt(space, &txn) != 0) > + struct txn_ro_savepoint svp; > + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) > return -1; > ssize_t count = index_count(index, itype, key, part_count); > if (count < 0) { > txn_rollback_stmt(); > return -1; > } > - txn_commit_ro_stmt(txn); > + txn_commit_ro_stmt(txn, &svp); > return count; > } > > @@ -381,7 +385,8 @@ box_index_iterator(uint32_t space_id, uint32_t index_id, int type, > if (key_validate(index->def, itype, key, part_count)) > return NULL; > struct txn *txn; > - if (txn_begin_ro_stmt(space, &txn) != 0) > + struct txn_ro_savepoint svp; > + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) > return NULL; > struct iterator *it = index_create_iterator(index, itype, > key, part_count); > @@ -389,7 +394,7 @@ box_index_iterator(uint32_t space_id, uint32_t index_id, int type, > txn_rollback_stmt(); > return NULL; > } > - txn_commit_ro_stmt(txn); > + txn_commit_ro_stmt(txn, &svp); > rmean_collect(rmean_box, IPROTO_SELECT, 1); > return it; > } > diff --git a/src/box/txn.h b/src/box/txn.h > index c366f3cab..822d5a7a9 100644 > --- a/src/box/txn.h > +++ b/src/box/txn.h > @@ -96,6 +96,20 @@ struct txn_savepoint { > struct stailq_entry *stmt; > }; > > +/** > + * Read-only transaction savepoint object. After completing a read-only > + * transaction, we must clear the region. However, if we just reset the region, > + * we may corrupt the data that was placed in the region before the read-only > + * transaction began. To avoid this, we should use truncation. This structure > + * contains the information required for truncation. > + */ > +struct txn_ro_savepoint { > + /** Region used during this transaction. */ > + struct region *region; > + /** Savepoint for region. */ > + size_t region_used; > +}; > + > extern double too_long_threshold; > > struct txn { > @@ -236,24 +250,27 @@ txn_begin_in_engine(struct engine *engine, struct txn *txn); > * select. > */ > static inline int > -txn_begin_ro_stmt(struct space *space, struct txn **txn) > +txn_begin_ro_stmt(struct space *space, struct txn **txn, > + struct txn_ro_savepoint *svp) > { > *txn = in_txn(); > if (*txn != NULL) { > struct engine *engine = space->engine; > return txn_begin_in_engine(engine, *txn); > } > + svp->region = &fiber()->gc; > + svp->region_used = region_used(svp->region); > return 0; > } > > static inline void > -txn_commit_ro_stmt(struct txn *txn) > +txn_commit_ro_stmt(struct txn *txn, struct txn_ro_savepoint *svp) > { > assert(txn == in_txn()); > if (txn) { > /* nothing to do */ > } else { > - fiber_gc(); > + region_truncate(svp->region, svp->region_used); > } > } > > -- > 2.25.0 >