From ad876049efa57969189b567a6b9738967b6887d8 Mon Sep 17 00:00:00 2001 From: Andre Branchizio Date: Thu, 9 Jul 2026 20:23:23 -0600 Subject: [PATCH] storage: resume bucket GC iterators after commits Bucket garbage collection commits after each full chunk and restarts the sharding-index iterator from the bucket prefix. On Vinyl, every restart can walk through the tombstones left by all preceding chunks before it reaches the next live tuple. The repeated prefix scan makes cleanup of a large bucket disproportionately expensive when the chunk is small. On Tarantool 2.11 and newer, retain the position of the last tuple committed by a full chunk (index:tuple_pos()) and pass it as the next iterator's `after` position. Tarantool keeps a position valid even after its tuple is deleted, so the next transaction resumes after the deleted tuple instead of rescanning the prefix. Pagination is a TREE-index feature, but functional and multikey TREE indexes still reject positions even though find_sharded_spaces() accepts them, so the index is probed once and GC falls back to restarting from the bucket prefix on older Tarantool versions and on unsupported indexes. Add coverage for the resumable cursor, the pagination-disabled fallback, and the unsupported-index fallback. Closes #661 NO_DOC=bugfix --- .../garbage_collector_test.lua | 156 ++++++++++++++++++ vshard/storage/init.lua | 20 ++- vshard/util.lua | 1 + 3 files changed, 175 insertions(+), 2 deletions(-) diff --git a/test/storage-luatest/garbage_collector_test.lua b/test/storage-luatest/garbage_collector_test.lua index c69dd593..1fa949d3 100644 --- a/test/storage-luatest/garbage_collector_test.lua +++ b/test/storage-luatest/garbage_collector_test.lua @@ -1074,3 +1074,159 @@ test_group.test_route_map_is_cleared = function(g) vtest.cluster_wait_vclock_all(g) g.replica_1_b:exec(bucket_set_protection, {true}) end + +-- +-- gh-661: bucket GC resumes after the last tuple deleted by the preceding +-- transaction on Tarantool versions with index pagination. Older versions and +-- indexes that do not support positions keep restarting from the bucket prefix. +-- +local function test_resumable_cursor_template(g, expected_resumes) + g.replica_1_a:exec(function(expected_resumes) + _G.bucket_gc_pause() + local s = box.space.test + local bucket_index = s.index.bucket_id + local bid = _G.get_first_bucket() + local old_chunk_size = ivconst.BUCKET_CHUNK_SIZE + local old_pairs = bucket_index.pairs + local chunk_size = 10 + local tuple_count = chunk_size * 2 + 5 + + -- Two full chunks and a non-empty tail make GC open three iterators. + -- Record whether each one resumed from a position (1) or restarted at + -- the bucket prefix (0). The first iterator always starts at the + -- prefix. The next two resume only on cores with index pagination. + -- 'after' is opaque, so its presence is the observable signal. + local resumes = {} + bucket_index.pairs = function(self, key, opts) + local after = opts and opts.after + table.insert(resumes, after ~= nil and 1 or 0) + if opts == nil then + return old_pairs(self, key) + end + return old_pairs(self, key, opts) + end + + local ok, err = pcall(function() + ivconst.BUCKET_CHUNK_SIZE = chunk_size + box.begin() + for i = 1, tuple_count do + s:replace{100000 + i, bid} + end + box.commit() + ivshard.storage.bucket_delete_garbage(bid, {force = true}) + ilt.assert_equals(bucket_index:count({bid}), 0) + ilt.assert_equals(resumes, expected_resumes) + end) + + bucket_index.pairs = old_pairs + ivconst.BUCKET_CHUNK_SIZE = old_chunk_size + _G.bucket_gc_continue() + ilt.assert(ok, err) + end, {expected_resumes}) + vtest.cluster_wait_vclock_all(g) +end + +-- +-- gh-661: pagination-capable cores resume the second and third iterators after +-- the preceding chunk instead of restarting at the bucket prefix. +-- +test_group.test_resumable_cursor_supported = function(g) + t.run_only_if(vutil.feature.index_pagination) + test_resumable_cursor_template(g, {0, 1, 1}) +end + +-- +-- gh-661: without index pagination (Tarantool < 2.11) every iterator restarts +-- at the bucket prefix. +-- +test_group.test_resumable_cursor_unsupported = function(g) + t.run_only_if(not vutil.feature.index_pagination) + test_resumable_cursor_template(g, {0, 0, 0}) +end + +-- +-- gh-661: functional and multikey sharding indexes report type == 'TREE' and +-- pass find_sharded_spaces(), but reject positions with "does not support +-- position by tuple"; hash indexes are not TREE at all. GC must fall back to +-- restarting from the bucket prefix for them and still delete the bucket. +-- +test_group.test_resumable_cursor_unsupported_index = function(g) + t.run_only_if(vutil.feature.index_pagination) + g.replica_1_a:exec(function() + _G.bucket_gc_pause() + local bid = _G.get_first_bucket() + local spaces = {} + local func_name = 'test_extract_bucket' + local old_chunk_size = ivconst.BUCKET_CHUNK_SIZE + + -- Each case fills exactly one chunk, so GC has to open a second + -- iterator after committing the delete. Cleanup succeeds only if an + -- index which cannot derive a position from a tuple falls back to + -- restarting at the bucket prefix. + local function run(name, space_opts, index_opts, tuple) + local space = box.schema.create_space(name, space_opts) + table.insert(spaces, space) + space:create_index('pk') + space:create_index('bucket_id', index_opts) + space:replace(tuple) + ivshard.storage.bucket_delete_garbage(bid, {force = true}) + ilt.assert_equals(space:count(), 0) + space:drop() + spaces[#spaces] = nil + end + + local ok, err = pcall(function() + ivconst.BUCKET_CHUNK_SIZE = 1 + + run('test_hash', { + engine = 'memtx', + format = {{'id', 'unsigned'}, {'bid', 'unsigned'}}, + }, { + type = 'hash', unique = true, parts = {2}, + }, {300000, bid}) + + run('test_multikey', { + engine = 'vinyl', + format = { + {'id', 'unsigned'}, + {'bid', 'unsigned'}, + {'tags', 'array'}, + }, + }, { + unique = false, + parts = { + {field = 2, type = 'unsigned'}, + {field = 3, type = 'string', path = '[*]'}, + }, + }, {300001, bid, {'tag'}}) + + -- Memtx MVCC does not support functional indexes. + if not box.cfg.memtx_use_mvcc_engine then + box.schema.func.create(func_name, { + body = [[function(tuple) return {tuple[2]} end]], + is_deterministic = true, + is_sandboxed = true, + }) + run('test_functional', { + engine = 'memtx', + format = {{'id', 'unsigned'}, {'bid', 'unsigned'}}, + }, { + unique = false, + func = func_name, + parts = {{field = 1, type = 'unsigned'}}, + }, {300002, bid}) + end + end) + + ivconst.BUCKET_CHUNK_SIZE = old_chunk_size + for _, space in ipairs(spaces) do + space:drop() + end + if box.func[func_name] ~= nil then + box.schema.func.drop(func_name) + end + _G.bucket_gc_continue() + ilt.assert(ok, err) + end) + vtest.cluster_wait_vclock_all(g) +end diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua index 3d619677..5ac6a660 100644 --- a/vshard/storage/init.lua +++ b/vshard/storage/init.lua @@ -2351,20 +2351,36 @@ end -- local function gc_bucket_in_space_xc(space, bucket_id, status) local bucket_index = space.index[lschema.shard_index] - if #bucket_index:select({bucket_id}, {limit = 1}) == 0 then + local first = bucket_index:select({bucket_id}, {limit = 1})[1] + if first == nil then return end local bucket_generation = M.bucket_generation local pk_parts = space.index[0].parts + -- A new iterator started at the bucket prefix has to walk past the + -- tombstones left by all preceding batches, which is expensive on Vinyl. + -- Resume from the position of the last committed tuple instead. The tuple + -- is deleted, but its position stays valid once the tuple is gone. + -- Pagination is a TREE-index feature, but functional and multikey TREE + -- indexes still reject positions with "does not support position by tuple" + -- even though find_sharded_spaces() accepts them. Probe the index once and + -- fall back to restarting from the bucket prefix when it is unsupported. + local use_cursor = util.feature.index_pagination and + bucket_index.type == 'TREE' and + pcall(bucket_index.tuple_pos, bucket_index, first) + local after ::restart:: local limit = consts.BUCKET_CHUNK_SIZE box.begin() M._on_bucket_event:run(consts.BUCKET_EVENT.GC, bucket_id, {spaces = {space.name}}) - for _, tuple in bucket_index:pairs({bucket_id}) do + for _, tuple in bucket_index:pairs({bucket_id}, {after = after}) do space:delete(util.tuple_extract_key(tuple, pk_parts)) limit = limit - 1 if limit == 0 then + if use_cursor then + after = bucket_index:tuple_pos(tuple) + end box.commit() bucket_generation = bucket_guard_xc(bucket_generation, bucket_id, status) diff --git a/vshard/util.lua b/vshard/util.lua index 6e80a524..78da3c98 100644 --- a/vshard/util.lua +++ b/vshard/util.lua @@ -438,6 +438,7 @@ end local feature = { msgpack_object = lmsgpack.object ~= nil, netbox_return_raw = version_is_at_least(2, 10, 0, 'beta', 2, 86), + index_pagination = version_is_at_least(2, 11, 0, nil, 0, 0), multilisten = luri.parse_many ~= nil, -- It appeared earlier but at 2.9.0 there is a strange bug about an immortal -- tuple - after :delete(pk) it still stays in the space. That makes the