Skip to content

Commit

Permalink
memtx: fix story delete statement list
Browse files Browse the repository at this point in the history
Current implementation of tracking statements that delete a story has a
flaw, consider the following example:

tx1('box.space.s:replace{0, 0}') -- statement 1

tx2('box.space.s:replace{0, 1}') -- statement 2
tx2('box.space.s:delete{0}') -- statement 3
tx2('box.space.s:replace{0, 2}') -- statement 4

When statement 1 is prepared, both statements 2 and 4 will be linked to the
delete statement list of {0, 0}'s story, though, apparently, statement 4
does not delete {0, 0}.

Let us notice the following: statement 4 is "pure" in the sense that, in
the transaction's scope, it is guaranteed not to replace any tuple — we
can retrieve this information when we check where the insert statement
violates replacement rules, use it to determine "pure" insert statements,
and skip them later on when, during preparation of insert statements, we
handle other insert statements which assume they do not replace anything
(i.e., have no visible old tuple).

On the contrary, statements 1 and 2 are "dirty": they assume that they
replaced nothing (i.e., there was no visible tuple in the index) — when one
of them gets prepared — the other one needs to be either aborted or
relinked to replace the prepared tuple.

We also need to fix relinking of delete statements from the older story
(in terms of the history chain) to the new one during preparation of insert
statements: a statement needs to be relinked iff it comes from a different
transaction (to be precise, there must, actually, be no more than one
delete statement from the same transaction).

Additionally, add assertions to verify the invariant that the story's
add (delete) psn is equal to the psn of the add (delete) statement's
transaction psn.

Closes tarantool#7214
Closes tarantool#7217

NO_DOC=bugfix
  • Loading branch information
CuriousGeorgiy committed Jul 6, 2022
1 parent 5557c47 commit cd2bd28
Show file tree
Hide file tree
Showing 7 changed files with 273 additions and 19 deletions.
4 changes: 4 additions & 0 deletions changelogs/unreleased/gh-7214-repeatable-replace-assertion.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## bugfix/memtx

* Fixed repeatable `replace` with _memtx_ transaction manager enabled which
triggered assertion (gh-7214).
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## bugfix/memtx

* Fixed repeatable `{in, up}sert` with _memtx_ transaction manager enabled which
caused spurious transaction conflict (gh-7217).
78 changes: 59 additions & 19 deletions src/box/memtx_tx.c
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,12 @@ memtx_tx_story_get(struct tuple *tuple)

mh_int_t pos = mh_history_find(txm.history, tuple, 0);
assert(pos != mh_end(txm.history));
return *mh_history_node(txm.history, pos);
struct memtx_story *story = *mh_history_node(txm.history, pos);
if (story->add_stmt != NULL)
assert(story->add_psn == story->add_stmt->txn->psn);
if (story->del_stmt != NULL)
assert(story->del_psn == story->del_stmt->txn->psn);
return story;
}

/**
Expand Down Expand Up @@ -1428,17 +1433,20 @@ memtx_tx_save_conflict(struct txn *breaker, struct txn *victim,
* Scan a history starting with @a story in @a index for a @a visible_tuple.
* If @a is_prepared_ok is true that prepared statements are visible for
* that lookup, and not visible otherwise.
*
* `is_own_change` is set to true iff `visible_tuple` was modified (either
* added or deleted) by `txn`.
*/
static void
memtx_tx_story_find_visible_tuple(struct memtx_story *story, struct txn *tnx,
uint32_t index, bool is_prepared_ok,
struct tuple **visible_tuple)
struct tuple **visible_tuple,
bool *is_own_change)
{
for (; story != NULL; story = story->link[index].older_story) {
assert(index < story->index_count);
bool unused;
if (memtx_tx_story_is_visible(story, tnx, is_prepared_ok,
visible_tuple, &unused))
visible_tuple, is_own_change))
return;
}
*visible_tuple = NULL;
Expand Down Expand Up @@ -1572,8 +1580,10 @@ check_dup_clean(struct txn_stmt *stmt, struct tuple *new_tuple,
*/
struct memtx_story *second_story = memtx_tx_story_get(replaced[i]);
struct tuple *check_visible;
bool unused;
memtx_tx_story_find_visible_tuple(second_story, txn, i,
true, &check_visible);
true, &check_visible,
&unused);

if (memtx_tx_check_dup(new_tuple, replaced[0], check_visible,
DUP_INSERT, space->index[i], space) != 0) {
Expand All @@ -1595,12 +1605,16 @@ check_dup_clean(struct txn_stmt *stmt, struct tuple *new_tuple,
* replace rules. See memtx_space_replace_all_keys comment.
* (!) Version for the case when replaced tuple is dirty.
* @return 0 on success or -1 on fail.
*
* `is_own_change` is set to true iff `old_tuple` was modified (either
* added or deleted) by `stmt`'s transaction.
*/
static int
check_dup_dirty(struct txn_stmt *stmt, struct tuple *new_tuple,
struct tuple **replaced, struct tuple **old_tuple,
enum dup_replace_mode mode,
struct memtx_tx_conflict **collected_conflicts)
struct memtx_tx_conflict **collected_conflicts,
bool *is_own_change)
{
assert(replaced[0] != NULL &&
tuple_has_flag(replaced[0], TUPLE_IS_DIRTY));
Expand All @@ -1610,7 +1624,7 @@ check_dup_dirty(struct txn_stmt *stmt, struct tuple *new_tuple,
struct memtx_story *old_story = memtx_tx_story_get(replaced[0]);
struct tuple *visible_replaced;
memtx_tx_story_find_visible_tuple(old_story, txn, 0, true,
&visible_replaced);
&visible_replaced, is_own_change);

if (memtx_tx_check_dup(new_tuple, *old_tuple, visible_replaced,
mode, space->index[0], space) != 0) {
Expand Down Expand Up @@ -1648,9 +1662,9 @@ check_dup_dirty(struct txn_stmt *stmt, struct tuple *new_tuple,

struct memtx_story *second_story = memtx_tx_story_get(replaced[i]);
struct tuple *check_visible;

bool unused;
memtx_tx_story_find_visible_tuple(second_story, txn, i, true,
&check_visible);
&check_visible, &unused);

if (memtx_tx_check_dup(new_tuple, visible_replaced,
check_visible, DUP_INSERT,
Expand All @@ -1673,12 +1687,16 @@ check_dup_dirty(struct txn_stmt *stmt, struct tuple *new_tuple,
* replace rules. See memtx_space_replace_all_keys comment.
* Call check_dup_clean or check_dup_dirty depending on situation.
* @return 0 on success or -1 on fail.
*
* `is_own_change` is set to true iff `old_tuple` was modified (either
* added or deleted) by `stmt`'s transaction.
*/
static int
check_dup_common(struct txn_stmt *stmt, struct tuple *new_tuple,
struct tuple **directly_replaced, struct tuple **old_tuple,
enum dup_replace_mode mode,
struct memtx_tx_conflict **collected_conflicts)
struct memtx_tx_conflict **collected_conflicts,
bool *is_own_change)
{
struct tuple *replaced = directly_replaced[0];
if (replaced == NULL || !tuple_has_flag(replaced, TUPLE_IS_DIRTY))
Expand All @@ -1688,7 +1706,7 @@ check_dup_common(struct txn_stmt *stmt, struct tuple *new_tuple,
else
return check_dup_dirty(stmt, new_tuple, directly_replaced,
old_tuple, mode,
collected_conflicts);
collected_conflicts, is_own_change);
}

static struct gap_item *
Expand Down Expand Up @@ -1837,9 +1855,10 @@ memtx_tx_history_add_insert_stmt(struct txn_stmt *stmt,
struct tuple *replaced = directly_replaced[0];

/* Check overwritten tuple */
bool is_own_change = false;
int rc = check_dup_common(stmt, new_tuple, directly_replaced,
&old_tuple, mode,
&collected_conflicts);
&collected_conflicts, &is_own_change);
if (rc != 0)
goto fail;

Expand Down Expand Up @@ -1909,7 +1928,8 @@ memtx_tx_history_add_insert_stmt(struct txn_stmt *stmt,
else
del_story = memtx_tx_story_get(old_tuple);
memtx_tx_story_link_deleted_by(del_story, stmt);
}
} else if (is_own_change)
stmt->is_pure_insert = true;

if (new_tuple != NULL) {
/*
Expand Down Expand Up @@ -2127,7 +2147,9 @@ memtx_tx_history_prepare_insert_stmt(struct txn_stmt *stmt)
struct txn_stmt *test_stmt = test->add_stmt;
if (test_stmt->txn == stmt->txn)
continue;
if (test_stmt->del_story != stmt->del_story) {
if (test_stmt->is_pure_insert)
continue;
if (test_stmt->del_story != NULL) {
assert(test_stmt->del_story->add_stmt->txn
== test_stmt->txn);
continue;
Expand All @@ -2154,11 +2176,17 @@ memtx_tx_history_prepare_insert_stmt(struct txn_stmt *stmt)
while (*from != NULL) {
struct txn_stmt *test_stmt = *from;
assert(test_stmt->del_story == old_story);
if (test_stmt == stmt || test_stmt->txn->psn != 0) {
/* This or prepared. Go to the next stmt. */
if (test_stmt->txn == stmt->txn) {
assert(test_stmt == stmt ||
test_stmt->add_story == NULL);
/*
* Statement from the same transaction. Go to
* the next statement.
*/
from = &test_stmt->next_in_del_list;
continue;
}
assert(test_stmt->txn->psn == 0);
/* Unlink from old list in any case. */
*from = test_stmt->next_in_del_list;
test_stmt->next_in_del_list = NULL;
Expand All @@ -2183,6 +2211,8 @@ memtx_tx_history_prepare_insert_stmt(struct txn_stmt *stmt)
struct txn_stmt *test_stmt = test->add_stmt;
if (test_stmt->txn == stmt->txn)
continue;
if (test_stmt->is_pure_insert)
continue;
if (test_stmt->del_story == story)
continue;
if (test_stmt->del_story != NULL &&
Expand Down Expand Up @@ -2254,11 +2284,17 @@ memtx_tx_history_prepare_delete_stmt(struct txn_stmt *stmt)
while (*itr != NULL) {
struct txn_stmt *test_stmt = *itr;
assert(test_stmt->del_story == story);
if (test_stmt == stmt && test_stmt->txn->psn != 0) {
/* This statement. Go to the next stmt. */
if (test_stmt->txn == stmt->txn) {
assert(test_stmt == stmt ||
test_stmt->add_story == NULL);
/*
* Statement from the same transaction. Go to the next
* statement.
*/
itr = &test_stmt->next_in_del_list;
continue;
}
assert(test_stmt->txn->psn == 0);
/* Unlink from old list in any case. */
*itr = test_stmt->next_in_del_list;
test_stmt->next_in_del_list = NULL;
Expand Down Expand Up @@ -2328,6 +2364,7 @@ memtx_tx_tuple_clarify_impl(struct txn *txn, struct space *space,
break;
if (story->add_psn != 0 && story->add_stmt != NULL &&
txn != NULL) {
assert(story->add_psn == story->add_stmt->txn->psn);
/*
* If we skip prepared story then the transaction
* must be before prepared in serialization order.
Expand All @@ -2340,6 +2377,7 @@ memtx_tx_tuple_clarify_impl(struct txn *txn, struct space *space,
story = story->link[index->dense_id].older_story;
}
if (story->del_psn != 0 && story->del_stmt != NULL && txn != NULL) {
assert(story->del_psn == story->del_stmt->txn->psn);
/*
* If we see a tuple that is deleted by prepared transaction
* then the transaction must be before prepared in serialization
Expand Down Expand Up @@ -2424,8 +2462,10 @@ memtx_tx_index_invisible_count_slow(struct txn *txn,

struct tuple *visible = NULL;
bool is_prepared_ok = detect_whether_prepared_ok(txn);
bool unused;
memtx_tx_story_find_visible_tuple(story, txn, index->dense_id,
is_prepared_ok, &visible);
is_prepared_ok, &visible,
&unused);
if (visible == NULL)
res++;
}
Expand Down
1 change: 1 addition & 0 deletions src/box/txn.c
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ txn_stmt_new(struct txn *txn)
stmt->row = NULL;
stmt->has_triggers = false;
stmt->does_require_old_tuple = false;
stmt->is_pure_insert = false;
return stmt;
}

Expand Down
8 changes: 8 additions & 0 deletions src/box/txn.h
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,14 @@ struct txn_stmt {
* old_tuple to be NULL.
*/
bool does_require_old_tuple;
/*
* `insert` statement is guaranteed not to delete anything
* from the transaction's point of view (i.e., there was a preceding
* `delete` in the scope of the same transaction): no linking to the
* list of `delete` statements is required during preparation of insert
* statements that add preceding stories.
*/
bool is_pure_insert;
/**
* Request type - IPROTO type code
*/
Expand Down
73 changes: 73 additions & 0 deletions test/box-luatest/gh_7214_repeatable_replace_assertion_test.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
local server = require('test.luatest_helpers.server')
local t = require('luatest')

local g = t.group()

g.before_all(function()
g.server = server:new{
alias = 'dflt',
box_cfg = {memtx_use_mvcc_engine = true}
}
g.server:start()
end)

g.after_all(function()
g.server:drop()
end)

g.after_each(function()
g.server:exec(function()
box.space.s:drop()
end)
end)

g.test_repeatable_replace_primary_idx = function()
g.server:exec(function()
local t = require('luatest')
local txn_proxy = require('test.box.lua.txn_proxy')

local s = box.schema.create_space('s')
s:create_index('pk')

local tx1 = txn_proxy:new()
local tx2 = txn_proxy:new()

tx1('box.begin()')
tx2('box.begin()')

tx1('box.space.s:replace{0, 0}')

tx2('box.space.s:replace{0, 1}')
tx2('box.space.s:delete{0}')
tx2('box.space.s:replace{0, 2}')

tx1('box.commit()')
t.assert_equals(tx2:commit(), "")
end)
end

g.test_repeatable_replace_secondary_idx = function()
g.server:exec(function()
local t = require('luatest')
local txn_proxy = require('test.box.lua.txn_proxy')

local s = box.schema.create_space('s')
s:create_index('pk')
s:create_index('sk', {parts = {2, 'unsigned'}})

local tx1 = txn_proxy:new()
local tx2 = txn_proxy:new()

tx1('box.begin()')
tx2('box.begin()')

tx1('box.space.s:replace{0, 0, 0}')

tx2('box.space.s:replace{0, 0, 1}')
tx2('box.space.s:delete{0}')
tx2('box.space.s:replace{0, 0, 2}')

tx1('box.commit()')
t.assert_equals(tx2:commit(), "")
end)
end
Loading

0 comments on commit cd2bd28

Please sign in to comment.