From 054526ac8472d005ae7468774f06290597278a1d Mon Sep 17 00:00:00 2001 From: Vladimir Davydov Date: Fri, 14 Jul 2023 11:25:49 +0300 Subject: [PATCH] box: allow to truncate temp and local spaces in ro mode To achieve that, we bypass the read-only check for the _truncate system space in box_process1() and perform it in the on_replace system trigger instead, when we know which space is truncated. Note, we have to move the check for insertion of a new record into the _truncate system space before the read-only check in the on_replace trigger callback; this is needed for initial recovery with a non-empty _truncate space to work. While we are at it, let's use recovery_state to make the check explicit. Closes #5616 @TarantoolBot document Title: Mention that temp and local spaces can be truncated in ro mode DML operations on temporary and local spaces can be performed even if the instance is in the read-only mode, but DDL operations (such as `alter`) are forbidden in this case[^1]. Technically, `truncate` is a DDL operation so initially it was forbidden as well. However, it should be safe to perform this operation on a temporary or local space because logically it only modifies the data stored in the space (like DML) and it isn't replicated (see tarantool/tarantool#4263). So starting from Tarantool 2.11.1 we allow users to truncate temporary spaces in the read-only mode. [^1]: https://www.tarantool.io/en/doc/latest/concepts/replication/repl_architecture/#replication-local --- .../gh-5616-temp-space-truncate-ro.md | 4 + src/box/alter.cc | 27 +-- src/box/box.cc | 12 +- src/box/box.h | 7 + .../gh_5616_temp_space_truncate_ro_test.lua | 167 ++++++++++++++++++ test/replication/anon.result | 4 - test/replication/anon.test.lua | 1 - 7 files changed, 204 insertions(+), 18 deletions(-) create mode 100644 changelogs/unreleased/gh-5616-temp-space-truncate-ro.md create mode 100644 test/box-luatest/gh_5616_temp_space_truncate_ro_test.lua diff --git a/changelogs/unreleased/gh-5616-temp-space-truncate-ro.md b/changelogs/unreleased/gh-5616-temp-space-truncate-ro.md new file mode 100644 index 000000000000..127aac42cecc --- /dev/null +++ b/changelogs/unreleased/gh-5616-temp-space-truncate-ro.md @@ -0,0 +1,4 @@ +## feature/box + +* Allowed truncation of temporary and local spaces in the read-only mode + (gh-5616). diff --git a/src/box/alter.cc b/src/box/alter.cc index 36d5c0477f1a..9aa186fc439a 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -2624,6 +2624,10 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event) /* Space drop - nothing to do. */ return 0; } + if (recovery_state == INITIAL_RECOVERY) { + /* Space creation during initial recovery - nothing to do. */ + return 0; + } uint32_t space_id; if (tuple_field_u32(new_tuple, BOX_TRUNCATE_FIELD_SPACE_ID, &space_id) != 0) @@ -2632,6 +2636,18 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event) if (old_space == NULL) return -1; + /* + * box_process1() bypasses the read-only check for the _truncate system + * space because there the space that is going to be truncated isn't yet + * known. Perform the check here if this statement was issued by this + * replica and the space isn't temporary or local. + */ + bool is_temp = space_is_temporary(old_space) || + space_is_local(old_space); + if (!is_temp && stmt->row->replica_id == 0 && + box_check_writable() != 0) + return -1; + /* * Check if a write privilege was given, return an error if not. * The check should precede initial recovery check to correctly @@ -2640,14 +2656,6 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event) if (access_check_space(old_space, PRIV_W) != 0) return -1; - if (stmt->row->type == IPROTO_INSERT) { - /* - * Space creation during initial recovery - - * nothing to do. - */ - return 0; - } - /* * System spaces use triggers to keep records in sync * with internal objects. Since space truncation doesn't @@ -2673,8 +2681,7 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event) * replication of local & temporary * spaces truncation. */ - if (space_is_temporary(old_space) || - space_is_local(old_space)) { + if (is_temp) { stmt->row->group_id = GROUP_LOCAL; /* * The trigger is invoked after txn->n_local_rows diff --git a/src/box/box.cc b/src/box/box.cc index 4a0536f7b7ad..8b37bda439da 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -347,7 +347,7 @@ box_check_slice_slow(void) return fiber_check_slice(); } -static int +int box_check_writable(void) { if (!is_ro_summary) @@ -3540,11 +3540,17 @@ box_process1(struct request *request, box_tuple_t **result) { if (box_check_slice() != 0) return -1; - /* Allow to write to temporary spaces in read-only mode. */ struct space *space = space_cache_find(request->space_id); if (space == NULL) return -1; - if (!space_is_temporary(space) && + /* + * Allow to write to temporary and local spaces in the read-only mode. + * To handle space truncation, we postpone the read-only check for the + * _truncate system space till the on_replace trigger is called, when + * we know which space is truncated. + */ + if (space_id(space) != BOX_TRUNCATE_ID && + !space_is_temporary(space) && !space_is_local(space) && box_check_writable() != 0) return -1; diff --git a/src/box/box.h b/src/box/box.h index 2bef05d97a38..5423289aa21c 100644 --- a/src/box/box.h +++ b/src/box/box.h @@ -146,6 +146,13 @@ box_check_slice(void) } } +/** + * Check if a write operation can be performed on this instance. + * Returns 0 on success. On error, sets diag and returns -1. + */ +int +box_check_writable(void); + void box_set_ro(void); diff --git a/test/box-luatest/gh_5616_temp_space_truncate_ro_test.lua b/test/box-luatest/gh_5616_temp_space_truncate_ro_test.lua new file mode 100644 index 000000000000..0dd944760fda --- /dev/null +++ b/test/box-luatest/gh_5616_temp_space_truncate_ro_test.lua @@ -0,0 +1,167 @@ +local server = require('luatest.server') +local t = require('luatest') + +local g_single = t.group('gh_5616_temp_space_truncate_ro.single') + +g_single.before_all(function(cg) + cg.server = server:new() + cg.server:start() +end) + +g_single.after_all(function(cg) + cg.server:drop() +end) + +g_single.after_each(function(cg) + cg.server:exec(function() + box.cfg({read_only = false}) + if box.space.test ~= nil then + box.space.test:drop() + end + end) +end) + +-- Checks that a temporary space can be truncated in the read-only mode. +g_single.test_temp_space_truncate_ro = function(cg) + cg.server:exec(function() + local s = box.schema.create_space('test', {temporary = true}) + s:create_index('primary') + s:insert({1}) + box.cfg({read_only = true}) + local ok, err = pcall(s.truncate, s) + t.assert(ok, err) + t.assert_equals(s:select(), {}) + end) +end + +-- Checks that a local space can be truncated in the read-only mode. +g_single.test_local_space_truncate_ro = function(cg) + cg.server:exec(function() + local s = box.schema.create_space('test', {is_local = true}) + s:create_index('primary') + s:insert({1}) + box.cfg({read_only = true}) + local ok, err = pcall(s.truncate, s) + t.assert(ok, err) + t.assert_equals(s:select(), {}) + end) +end + +-- Checks that a persistent space can't be truncated in the read-only mode. +g_single.test_persistent_space_truncate_ro = function(cg) + cg.server:exec(function() + local s = box.schema.create_space('test') + s:create_index('primary') + s:insert({1}) + box.cfg({read_only = true}) + local ok, err = pcall(s.truncate, s) + t.assert_not(ok, err) + t.assert_equals(s:select(), {{1}}) + end) +end + +local g_replication = t.group('gh_5616_temp_space_truncate_ro.replication') + +g_replication.before_all(function(cg) + cg.master = server:new({alias = 'master'}) + cg.master:start() + cg.replica = server:new({ + alias = 'replica', + box_cfg = { + read_only = true, + replication = cg.master.net_box_uri, + }, + }) + cg.replica:start() +end) + +g_replication.after_all(function(cg) + cg.replica:drop() + cg.master:drop() +end) + +g_replication.after_each(function(cg) + cg.master:exec(function() + if box.space.test ~= nil then + box.space.test:drop() + end + end) + cg.replica:wait_for_vclock_of(cg.master) +end) + +-- Checks that a truncate operation for a temporary space isn't replicated to +-- a read-only replica. +g_replication.test_temp_space_truncate_ro = function(cg) + cg.master:exec(function() + local s = box.schema.create_space('test', {temporary = true}) + s:create_index('primary') + s:insert({1}) + end) + cg.replica:wait_for_vclock_of(cg.master) + cg.replica:exec(function() + local s = box.space.test + t.assert_equals(s:select(), {}) + s:insert({2}) + end) + cg.master:exec(function() + local s = box.space.test + s:truncate() + t.assert_equals(s:select(), {}) + end) + cg.replica:wait_for_vclock_of(cg.master) + cg.replica:exec(function() + local s = box.space.test + t.assert_equals(s:select(), {{2}}) + end) +end + +-- Checks that a truncate operation for a local space isn't replicated to +-- a read-only replica. +g_replication.test_local_space_truncate_ro = function(cg) + cg.master:exec(function() + local s = box.schema.create_space('test', {is_local = true}) + s:create_index('primary') + s:insert({1}) + end) + cg.replica:wait_for_vclock_of(cg.master) + cg.replica:exec(function() + local s = box.space.test + t.assert_equals(s:select(), {}) + s:insert({2}) + end) + cg.master:exec(function() + local s = box.space.test + s:truncate() + t.assert_equals(s:select(), {}) + end) + cg.replica:wait_for_vclock_of(cg.master) + cg.replica:exec(function() + local s = box.space.test + t.assert_equals(s:select(), {{2}}) + end) +end + +-- Checks that a truncate operation for a persistent space is replicated to +-- a read-only replica. +g_replication.test_persistent_space_truncate_ro = function(cg) + cg.master:exec(function() + local s = box.schema.create_space('test') + s:create_index('primary') + s:insert({1}) + end) + cg.replica:wait_for_vclock_of(cg.master) + cg.replica:exec(function() + local s = box.space.test + t.assert_equals(s:select(), {{1}}) + end) + cg.master:exec(function() + local s = box.space.test + s:truncate() + t.assert_equals(s:select(), {}) + end) + cg.replica:wait_for_vclock_of(cg.master) + cg.replica:exec(function() + local s = box.space.test + t.assert_equals(s:select(), {}) + end) +end diff --git a/test/replication/anon.result b/test/replication/anon.result index 4dfadd40056e..68e629f61b10 100644 --- a/test/replication/anon.result +++ b/test/replication/anon.result @@ -143,10 +143,6 @@ box.space.loc:drop() | --- | - error: Can't modify data on a read-only instance - box.cfg.read_only is true | ... -box.space.loc:truncate() - | --- - | - error: Can't modify data on a read-only instance - box.cfg.read_only is true - | ... test_run:cmd('switch default') | --- diff --git a/test/replication/anon.test.lua b/test/replication/anon.test.lua index c0b988eef988..97b2e7d67f9b 100644 --- a/test/replication/anon.test.lua +++ b/test/replication/anon.test.lua @@ -47,7 +47,6 @@ box.cfg{read_only=false} box.space.test:insert{2} box.space.loc:drop() -box.space.loc:truncate() test_run:cmd('switch default')