commit 1babcf1ea49d9c8823a706a36d1a12ef43733d28 from: Ilya Verbin via: Aleksandr Lyapunov date: Fri Oct 13 09:45:32 2023 UTC box: fix space:bsize() handling on space alter During building an index in background, some transaction can perform a dml request that affects space size (e.g. a replace), but the size will remain the same, because bsize is moved from the old space to the new space in memtx_space_prepare_alter() prior to space_execute_dml(). Fix this issue by calling space_finish_alter() in alter_space_do(). In fact, this patch partially reverts commit 9ec3b1a445a6 ("alter: zap space_vtab::commit_alter"). NO_DOC=bugfix Closes #9247 (cherry picked from commit 54a42186de2776b4fa970ee1a9cf53c543a1cb26) commit - 5e147360b6570eaad2a81221bfaa4191cf23075d commit + 1babcf1ea49d9c8823a706a36d1a12ef43733d28 blob - /dev/null blob + e11362fca6e2c9854e898458ef32123d6ec26dd6 (mode 644) --- /dev/null +++ changelogs/unreleased/gh-9247-fix-space-bsize-handling-on-space-alter.md @@ -0,0 +1,4 @@ +## bugfix/core + +* Fixed a bug that could result in the incorrect `space:bsize()` when altering + a primary index concurrently with DML operations (gh-9247). blob - 51abc28fe75cf7ecb7e398ffa27205b67bc7fc1e blob + 8d8d40a2b839310d213a0de3e827a7158bdc5884 --- src/box/alter.cc +++ src/box/alter.cc @@ -1017,6 +1017,7 @@ alter_space_do(struct txn_stmt *stmt, struct alter_spa * The new space is ready. Time to update the space * cache with it. */ + space_finish_alter(alter->old_space, alter->new_space); space_cache_replace(alter->old_space, alter->new_space); space_detach_constraints(alter->old_space); space_unpin_collations(alter->old_space); blob - 63b102aae370a9d68790ef38e289cc4b57704065 blob + 83e1d8bb96d6106f2d86e436a651823dea629f9a --- src/box/blackhole.c +++ src/box/blackhole.c @@ -126,6 +126,7 @@ static const struct space_vtab blackhole_space_vtab = /* .build_index = */ generic_space_build_index, /* .swap_index = */ generic_space_swap_index, /* .prepare_alter = */ generic_space_prepare_alter, + /* .finish_alter = */ generic_space_finish_alter, /* .prepare_upgrade = */ generic_space_prepare_upgrade, /* .invalidate = */ generic_space_invalidate, }; blob - e36d5bf6e6cfaf2c0281b58f94909480b886b600 blob + 3f6594bd65809af3e3481ee1c5f3daebdd790d39 --- src/box/memtx_space.c +++ src/box/memtx_space.c @@ -1052,7 +1052,6 @@ memtx_space_drop_primary_key(struct space *space) * can be put back online properly. */ memtx_space->replace = memtx_space_replace_no_keys; - memtx_space->bsize = 0; } static void @@ -1369,8 +1368,24 @@ memtx_space_prepare_alter(struct space *old_space, str } new_memtx_space->replace = old_memtx_space->replace; - new_memtx_space->bsize = old_memtx_space->bsize; return 0; +} + +/** + * Copy bsize to the newly altered space from the old space. + * In case of DropIndex or TruncateIndex alter operations, the new space will be + * empty, and bsize must not be copied. + */ +static void +memtx_space_finish_alter(struct space *old_space, struct space *new_space) +{ + struct memtx_space *old_memtx_space = (struct memtx_space *)old_space; + struct memtx_space *new_memtx_space = (struct memtx_space *)new_space; + + bool is_empty = new_space->index_count == 0 || + index_size(new_space->index[0]) == 0; + if (!is_empty) + new_memtx_space->bsize = old_memtx_space->bsize; } /* }}} DDL */ @@ -1395,6 +1410,7 @@ static const struct space_vtab memtx_space_vtab = { /* .build_index = */ memtx_space_build_index, /* .swap_index = */ generic_space_swap_index, /* .prepare_alter = */ memtx_space_prepare_alter, + /* .finish_alter = */ memtx_space_finish_alter, /* .prepare_upgrade = */ memtx_space_prepare_upgrade, /* .invalidate = */ generic_space_invalidate, }; blob - 66fa9fc8307e103c6cd5abbdfeb9c3cafeb3da57 blob + 9c38cd3f96707affa936afd3a096558ec70100b1 --- src/box/session_settings.c +++ src/box/session_settings.c @@ -437,6 +437,7 @@ const struct space_vtab session_settings_space_vtab = /* .build_index = */ generic_space_build_index, /* .swap_index = */ generic_space_swap_index, /* .prepare_alter = */ generic_space_prepare_alter, + /* .finish_alter = */ generic_space_finish_alter, /* .prepare_upgrade = */ generic_space_prepare_upgrade, /* .invalidate = */ generic_space_invalidate, }; blob - b7fbf6fdbc5bf10c0248031218e2e03e20ba9bc4 blob + 4a2a51b7ddef415fd5b70d235453cbffe176d044 --- src/box/space.c +++ src/box/space.c @@ -971,6 +971,13 @@ generic_space_prepare_alter(struct space *old_space, s return 0; } +void +generic_space_finish_alter(struct space *old_space, struct space *new_space) +{ + (void)old_space; + (void)new_space; +} + int generic_space_prepare_upgrade(struct space *old_space, struct space *new_space) { blob - 9a101caf0ec7937cb7f53f2653b4fe0d114d3db2 blob + 930184cf329d1320f698f88a0a6a3c3e4542fb9b --- src/box/space.h +++ src/box/space.h @@ -144,6 +144,12 @@ struct space_vtab { */ int (*prepare_alter)(struct space *old_space, struct space *new_space); + /** + * Notify the engine after altering a space and before replacing + * old_space with new_space in the space cache. + */ + void (*finish_alter)(struct space *old_space, + struct space *new_space); /** Prepares a space for online upgrade on alter. */ int (*prepare_upgrade)(struct space *old_space, struct space *new_space); @@ -561,6 +567,13 @@ space_prepare_alter(struct space *old_space, struct sp { assert(old_space->vtab == new_space->vtab); return new_space->vtab->prepare_alter(old_space, new_space); +} + +static inline void +space_finish_alter(struct space *old_space, struct space *new_space) +{ + assert(old_space->vtab == new_space->vtab); + new_space->vtab->finish_alter(old_space, new_space); } static inline int @@ -668,6 +681,7 @@ int generic_space_check_format(struct space *, struct int generic_space_build_index(struct space *, struct index *, struct tuple_format *, bool); int generic_space_prepare_alter(struct space *, struct space *); +void generic_space_finish_alter(struct space *, struct space *); int generic_space_prepare_upgrade(struct space *old_space, struct space *new_space); void generic_space_invalidate(struct space *); blob - 6b3dfd790ddccc36be26d4badafce661e0dd4a5a blob + 8b655b291df2fe1e940da25acf08e1e3fccab3fd --- src/box/sysview.c +++ src/box/sysview.c @@ -505,6 +505,7 @@ static const struct space_vtab sysview_space_vtab = { /* .build_index = */ generic_space_build_index, /* .swap_index = */ generic_space_swap_index, /* .prepare_alter = */ generic_space_prepare_alter, + /* .finish_alter = */ generic_space_finish_alter, /* .prepare_upgrade = */ generic_space_prepare_upgrade, /* .invalidate = */ generic_space_invalidate, }; blob - 2cb41a222d8d5e87b8bc93e393f57ea9e6253e46 blob + 259c1b9f682b8490eb5281e8f70cee39eb38f189 --- src/box/vinyl.c +++ src/box/vinyl.c @@ -4641,6 +4641,7 @@ static const struct space_vtab vinyl_space_vtab = { /* .build_index = */ vinyl_space_build_index, /* .swap_index = */ vinyl_space_swap_index, /* .prepare_alter = */ vinyl_space_prepare_alter, + /* .finish_alter = */ generic_space_finish_alter, /* .prepare_upgrade = */ generic_space_prepare_upgrade, /* .invalidate = */ vinyl_space_invalidate, }; blob - /dev/null blob + 5fe9a9bdf6f2e12959616387a5981d1c3e870748 (mode 644) --- /dev/null +++ test/box-luatest/gh_9247_bsize_on_concurrent_dml_and_alter_test.lua @@ -0,0 +1,118 @@ +local server = require('luatest.server') +local t = require('luatest') +local g = t.group('gh-9247') + +g.before_all(function(cg) + 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.space.test:drop() + end) +end) + +-- Based on `test/box/alter-primary-index-tuple-leak-long.test.lua'. +local function concurrent_insert_and_alter_helper(cg, rollback) + cg.server:exec(function(rollback) + local fiber = require('fiber') + local s = box.schema.space.create('test') + local idx = s:create_index('pk') + local alter_started = false + local data = string.rep('a', 66) + + -- Create a table with enough tuples to make primary index alter in + -- background. + for i = 0, 7000, 100 do + box.begin() + for j = i, i + 99 do + s:insert{j, j, data} + end + box.commit() + end + + -- Wait for fiber yield during altering and insert some tuples. + local fib_insert = fiber.new(function() + while not alter_started do + fiber.sleep(0) + end + box.begin() + for i = 8000, 9000 do + s:insert{i, i, data} + end + box.commit() + end) + + -- Alter primary index. + -- If the index will not be altered in background, the test will fail + -- after timeout (fib_insert:join() will wait forever). + local fib_alter = fiber.new(function() + alter_started = true + box.begin() + idx:alter{parts = {{field = 2}}} + if rollback then + box.rollback() + else + box.commit() + end + alter_started = false + end) + + fib_insert:set_joinable(true) + fib_alter:set_joinable(true) + fib_insert:join() + fib_alter:join() + + local bsize = 0 + for _, tuple in s:pairs() do + bsize = bsize + tuple:bsize() + end + t.assert_equals(s:bsize(), bsize) + end, {rollback}) +end + +-- Check that space:bsize() is correct for a built-in-background primary index +-- with concurrent inserts. +g.test_concurrent_insert_and_alter_commit = function(cg) + concurrent_insert_and_alter_helper(cg, false) +end + +-- Check that space:bsize() is correct for a built-in-background primary index +-- with concurrent inserts. +g.test_concurrent_insert_and_alter_rollback = function(cg) + concurrent_insert_and_alter_helper(cg, true) +end + +local function truncate_helper(cg, rollback) + cg.server:exec(function(rollback) + local s = box.schema.space.create('test') + s:create_index('pk') + s:insert{1, 2, 3} + local bsize_before_truncate = s:bsize() + + box.begin() + s:truncate() + if rollback then + box.rollback() + t.assert_equals(s:bsize(), bsize_before_truncate) + else + box.commit() + t.assert_equals(s:bsize(), 0) + end + end, {rollback}) +end + +-- Check that space:bsize() is correct after successful space:truncate(). +g.test_truncate_commit = function(cg) + truncate_helper(cg, false) +end + +-- Check that space:bsize() is correct after rolled back space:truncate(). +g.test_truncate_rollback = function(cg) + truncate_helper(cg, true) +end