[Tarantool-patches] [PATCH] box: do not reset region on select
Alexander Turenko
alexander.turenko at tarantool.org
Tue Dec 29 04:46:35 MSK 2020
From: Mergen Imeev <imeevma at gmail.com>
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
More information about the Tarantool-patches
mailing list