diff --git a/changelogs/unreleased/gh-10895-fix-vinyl-deferred-delete-compaction.md b/changelogs/unreleased/gh-10895-fix-vinyl-deferred-delete-compaction.md new file mode 100644 index 0000000000000000000000000000000000000000..63226d6cc3ab7040008696c9179ab66c3386ef90 --- /dev/null +++ b/changelogs/unreleased/gh-10895-fix-vinyl-deferred-delete-compaction.md @@ -0,0 +1,4 @@ +## bugfix/vinyl + +* Fixed a bug when a deleted secondary index key wasn't purged on compaction + of a space with the `defer_deletes` option enabled (gh-10895). diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c index da3b6c0c2a7c7807535cf87f1da9d871f8c92a78..1ede93a2855f1c37292c679c328f83cdd5a5f944 100644 --- a/src/box/vy_write_iterator.c +++ b/src/box/vy_write_iterator.c @@ -843,10 +843,17 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct vy_entry prev, /* * Optimization 4: discard a DELETE statement referenced * by a read view if it is preceded by another DELETE for - * the same key. + * the same key unless the latter is marked as SKIP READ + * (i.e. it's a deferred DELETE). Deferred DELETEs are + * dumped out of order so there may be a statement with + * the LSN lying between the two DELETEs in an older source + * not included into this compaction task. If we discarded + * the newer DELETE, we wouldn't purge this statement on + * major compaction. */ if (prev.stmt != NULL && vy_stmt_type(prev.stmt) == IPROTO_DELETE && + (vy_stmt_flags(prev.stmt) & VY_STMT_SKIP_READ) == 0 && vy_stmt_type(h->entry.stmt) == IPROTO_DELETE) { vy_write_history_destroy(h); rv->history = NULL; diff --git a/test/unit/vy_iterators_helper.h b/test/unit/vy_iterators_helper.h index a3f7edf430f2bf5bfb9b3108a1af998ad8d1434e..7707d13b0898acbfba5cf0846275446f35343f2a 100644 --- a/test/unit/vy_iterators_helper.h +++ b/test/unit/vy_iterators_helper.h @@ -54,6 +54,9 @@ #define STMT_TEMPLATE_DEFERRED_DELETE(lsn, type, ...) \ STMT_TEMPLATE_FLAGS(lsn, type, VY_STMT_DEFERRED_DELETE, __VA_ARGS__) +#define STMT_TEMPLATE_SKIP_READ(lsn, type, ...) \ +STMT_TEMPLATE_FLAGS(lsn, type, VY_STMT_SKIP_READ, __VA_ARGS__) + extern struct vy_stmt_env stmt_env; extern struct vy_mem_env mem_env; extern struct vy_cache_env cache_env; diff --git a/test/unit/vy_write_iterator.c b/test/unit/vy_write_iterator.c index 5bce176191665993f8f5fd755210d500253ec2ad..7a85d649ce4d39eb11419a82f79ab4dcf5dcc520 100644 --- a/test/unit/vy_write_iterator.c +++ b/test/unit/vy_write_iterator.c @@ -150,7 +150,7 @@ void test_basic(void) { header(); - plan(58); + plan(61); { /* * STATEMENT: REPL REPL REPL DEL REPL REPL REPL REPL REPL REPL @@ -396,6 +396,36 @@ test_basic(void) expected, expected_count, NULL, 0, vlsns, vlsns_count, true, false); } + +/* + * STATEMENT: REPL DEL REPL DEL REPL DEL + * LSN: 4 5 10 11 15 20 + * SKIP READ: + + * READ VIEW: * * + * \_______/\_______/\_______/ + * merge merge skip + * + * is_last_level = false + * + * Check that a tautological DELETE isn't skipped if the preceding DELETE + * referenced by an older read view was deferred (marked as SKIP READ). + */ +{ + const struct vy_stmt_template content[] = { + STMT_TEMPLATE(4, REPLACE, 1, 1), + STMT_TEMPLATE_SKIP_READ(5, DELETE, 1), + STMT_TEMPLATE(10, REPLACE, 1, 2), + STMT_TEMPLATE(11, DELETE, 1), + STMT_TEMPLATE(15, REPLACE, 1, 3), + STMT_TEMPLATE(20, DELETE, 1), + }; + const struct vy_stmt_template expected[] = { content[3], content[1] }; + const int vlsns[] = {5, 11}; + compare_write_iterator_results(content, lengthof(content), + expected, lengthof(expected), NULL, 0, + vlsns, lengthof(vlsns), true, false); +} + { /* * STATEMENT: INS DEL REPL DEL REPL REPL INS REPL diff --git a/test/vinyl-luatest/gh_10895_deferred_delete_compaction_test.lua b/test/vinyl-luatest/gh_10895_deferred_delete_compaction_test.lua new file mode 100644 index 0000000000000000000000000000000000000000..0afb148d34606b4dfdb9775f65cb2fc26e0d9bbe --- /dev/null +++ b/test/vinyl-luatest/gh_10895_deferred_delete_compaction_test.lua @@ -0,0 +1,113 @@ +local server = require('luatest.server') +local t = require('luatest') + +local g = t.group() + +g.before_all(function(cg) + t.tarantool.skip_if_not_debug() + cg.server = server:new() + cg.server:start() +end) + +g.after_all(function(cg) + cg.server:drop() +end) + +g.after_each(function(cg) + cg.server:exec(function() + box.error.injection.set('ERRINJ_VY_COMPACTION_DELAY', false) + if box.space.test ~= nil then + box.space.test:drop() + end + end) +end) + +g.test_deferred_delete_compaction = function(cg) + cg.server:exec(function() + local fiber = require('fiber') + + local s = box.schema.space.create('test', { + engine = 'vinyl', + defer_deletes = true, + }) + s:create_index('primary') + s:create_index('secondary', { + unique = false, + parts = {2, 'unsigned'}, + }) + + -- Temporarily block compaction. + box.error.injection.set('ERRINJ_VY_COMPACTION_DELAY', true) + + -- Create primary and secondary index run files with INSERT{1,10}. + s:insert({1, 10}) + box.snapshot() + + -- Create the primary index run file with DELETE{1}. + -- + -- Generation of DELETE{1,10} for the secondary index is deferred + -- until the primary index compaction. + s:delete({1}) + box.snapshot() + + -- Trigger primary index compaction (it's still blocked though). + s.index.primary:compact() + + -- Create primary and secondary index run files with INSERT{1,10}. + s:upsert({1, 10}, {}) + box.snapshot() + + -- Create a read view referring to the last INSERT{1,10}. + local f = fiber.create(function() + box.begin() + s:select() + fiber.sleep(9000) + end) + fiber.sleep(0.1) + + -- Unblock compaction and wait for it to complete. + -- + -- Compaction of the primary index generates DELETE{1, 10} for + -- the older INSERT{1, 10} stored in the secondary index. + box.error.injection.set('ERRINJ_VY_COMPACTION_DELAY', false) + while box.stat.vinyl().scheduler.tasks_inprogress > 0 do + fiber.sleep(0.1) + end + + -- Write DELETE{1,10} + INSERT{1,20} to the secondary index. + -- + -- Now, the memory level of the secondary index contains two + -- DELETE{1,10} statements: the first one is generated by the primary + -- index compaction for the older INSERT; the second one is generated + -- by the UPSERT for the newer INSERT. The newer DELETE should + -- overwrite the older one, but due to gh-10895 the older DELETE + -- overwrites the newer one, as a result the newer INSERT was never + -- purged. + s:upsert({1, 20}, {{'=', 2, 20}}) + box.snapshot() + + f:cancel() + + -- Delete INSERT{1,20}, trigger primary index compaction to generate + -- DELETE for the secondary index, then dump and compaction of the + -- secondary index. + s:delete({1}) + box.snapshot() + s.index.primary:compact() + while box.stat.vinyl().scheduler.tasks_inprogress > 0 do + fiber.sleep(0.1) + end + + box.snapshot() + s.index.secondary:compact() + while box.stat.vinyl().scheduler.tasks_inprogress > 0 do + fiber.sleep(0.1) + end + + -- No statements should be left in either of the indexes, but if + -- gh-10895 wasn't fixed, the secondary index run file would still + -- contain the newer INSERT{1,10}. + t.assert_equals(s.index.primary:len(), 0) + t.assert_equals(s.index.secondary:len(), 0) + end) +end