From 074e78dd07414f1974d740854f7981b4c4064a43 Mon Sep 17 00:00:00 2001 From: Georgy Moiseev Date: Tue, 12 Mar 2024 18:36:49 +0300 Subject: [PATCH] crud: fix passing errors from storages for merger Before this patch, returning any errors from storages for merger operations (`crud.select`, `crud.pairs`, `readview:select`, `readview:pairs`) had resulted in non-informative "Invalid merge source 0x556bb6885a00" errors. This patch fixes the issue by replacing error return with error throw. Part of #373 --- CHANGELOG.md | 2 + crud/select.lua | 8 +-- test/integration/pairs_readview_test.lua | 21 +++++++- test/integration/pairs_test.lua | 21 +++++++- test/integration/read_scenario.lua | 66 ++++++++++++++++++++++- test/integration/select_readview_test.lua | 20 ++++++- test/integration/select_test.lua | 21 +++++++- 7 files changed, 147 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc81dec2..5bad9343 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. non-iterating indexes (#373). * Precision loss for decimal conditions in case of non-indexed fields or non-iterating indexes (#373). +* Passing errors from storages for merger operations (`crud.select`, + `crud.pairs`, `readview:select`, `readview:pairs`) (#423). ## [1.4.3] - 05-02-24 diff --git a/crud/select.lua b/crud/select.lua index dc53062a..8c609dac 100644 --- a/crud/select.lua +++ b/crud/select.lua @@ -61,12 +61,12 @@ local function select_on_storage(space_name, index_id, conditions, opts) local space = box.space[space_name] if space == nil then - return cursor, SelectError:new("Space %q doesn't exist", space_name) + SelectError:assert(false, "Space %q doesn't exist", space_name) end local index = space.index[index_id] if index == nil then - return cursor, SelectError:new("Index with ID %s doesn't exist", index_id) + SelectError:assert(false, "Index with ID %s doesn't exist", index_id) end local _, err = sharding.check_sharding_hash(space_name, @@ -83,7 +83,7 @@ local function select_on_storage(space_name, index_id, conditions, opts) scan_condition_num = opts.scan_condition_num, }) if err ~= nil then - return cursor, SelectError:new("Failed to generate tuples filter: %s", err) + return SelectError:assert(false, "Failed to generate tuples filter: %s", err) end -- execute select @@ -95,7 +95,7 @@ local function select_on_storage(space_name, index_id, conditions, opts) yield_every = opts.yield_every, }) if err ~= nil then - return cursor, SelectError:new("Failed to execute select: %s", err) + return SelectError:assert(false, "Failed to execute select: %s", err) end if resp.tuples_fetched < opts.limit or opts.limit == 0 then diff --git a/test/integration/pairs_readview_test.lua b/test/integration/pairs_readview_test.lua index cc1fe856..b19b364c 100644 --- a/test/integration/pairs_readview_test.lua +++ b/test/integration/pairs_readview_test.lua @@ -895,11 +895,14 @@ local function read_impl(cg, space, conditions, opts) local status, resp = pcall(function() return rv:pairs(space, conditions, opts):totable() end) - t.assert(status, resp) rv:close() - return resp, nil + if status then + return resp, nil + else + return nil, resp + end end, {space, conditions, opts}) end @@ -919,3 +922,17 @@ for case_name_template, case in pairs(gh_373_types_cases) do case(g, read_impl) end end + +pgroup.before_test( + 'test_pairs_merger_process_storage_error', + read_scenario.before_merger_process_storage_error +) + +pgroup.test_pairs_merger_process_storage_error = function(g) + read_scenario.merger_process_storage_error(g, read_impl) +end + +pgroup.after_test( + 'test_pairs_merger_process_storage_error', + read_scenario.after_merger_process_storage_error +) diff --git a/test/integration/pairs_test.lua b/test/integration/pairs_test.lua index 097784a0..ddba820f 100644 --- a/test/integration/pairs_test.lua +++ b/test/integration/pairs_test.lua @@ -905,9 +905,12 @@ local function read_impl(cg, space, conditions, opts) local status, resp = pcall(function() return crud.pairs(space, conditions, opts):totable() end) - t.assert(status, resp) - return resp, nil + if status then + return resp, nil + else + return nil, resp + end end, {space, conditions, opts}) end @@ -927,3 +930,17 @@ for case_name_template, case in pairs(gh_373_types_cases) do case(g, read_impl) end end + +pgroup.before_test( + 'test_pairs_merger_process_storage_error', + read_scenario.before_merger_process_storage_error +) + +pgroup.test_pairs_merger_process_storage_error = function(g) + read_scenario.merger_process_storage_error(g, read_impl) +end + +pgroup.after_test( + 'test_pairs_merger_process_storage_error', + read_scenario.after_merger_process_storage_error +) diff --git a/test/integration/read_scenario.lua b/test/integration/read_scenario.lua index 493c93be..62dc8b76 100644 --- a/test/integration/read_scenario.lua +++ b/test/integration/read_scenario.lua @@ -50,11 +50,12 @@ local function gh_418_read_with_secondary_noneq_index_condition(cg, read) -- iterator had erroneously expected tuples to be sorted by `last_login` -- index while iterating on `city` index. Before the issue had beed fixed, -- user had received only one record instead of two. - local result = read(cg, + local result, err = read(cg, 'logins', {{'=', 'city', 'Tatsumi Port Island'}, {'<=', 'last_login', 42}}, {bucket_id = PINNED_BUCKET_NO} ) + t.assert_equals(err, nil) if type(result) == 'number' then -- crud.count t.assert_equals(result, 2) @@ -555,8 +556,71 @@ for space_kind, space_option in pairs(datetime_condition_space_options) do end +local function before_merger_process_storage_error(cg) + helpers.call_on_storages(cg.cluster, function(server) + server:exec(function() + local space + if box.info.ro == false then + space = box.schema.space.create('speedy_gonzales', {if_not_exists = true}) + + space:format({ + {name = 'id', type = 'unsigned'}, + {name = 'bucket_id', type = 'unsigned'}, + }) + + space:create_index('pk', { + parts = {'id'}, + if_not_exists = true, + }) + + space:create_index('bucket_id', { + parts = {'bucket_id'}, + unique = false, + if_not_exists = true, + }) + end + + local real_select_impl = rawget(_G, '_crud').select_on_storage + rawset(_G, '_real_select_impl', real_select_impl) + + -- Drop right before select to cause storage-side error. + -- Work guaranteed only with mode = 'write'. + local function erroneous_select_impl(...) + if box.info.ro == false then + space:drop() + end + + return real_select_impl(...) + end + + rawget(_G, '_crud').select_on_storage = erroneous_select_impl + end) + end) +end + +local function merger_process_storage_error(cg, read) + local _, err = read(cg, 'speedy_gonzales', {{'==', 'id', 1}}) + t.assert_not_equals(err, nil) + + local err_msg = err.err or tostring(err) + t.assert_str_contains(err_msg, "Space \"speedy_gonzales\" doesn't exist") +end + +local function after_merger_process_storage_error(cg) + helpers.call_on_storages(cg.cluster, function(server) + server:exec(function() + local real_select_impl = rawget(_G, '_real_select_impl') + + rawget(_G, '_crud').select_on_storage = real_select_impl + end) + end) +end + return { gh_418_read_with_secondary_noneq_index_condition = gh_418_read_with_secondary_noneq_index_condition, gh_373_read_with_decimal_condition_cases = gh_373_read_with_decimal_condition_cases, gh_373_read_with_datetime_condition_cases = gh_373_read_with_datetime_condition_cases, + before_merger_process_storage_error = before_merger_process_storage_error, + merger_process_storage_error = merger_process_storage_error, + after_merger_process_storage_error = after_merger_process_storage_error, } diff --git a/test/integration/select_readview_test.lua b/test/integration/select_readview_test.lua index 316e5280..72f61988 100644 --- a/test/integration/select_readview_test.lua +++ b/test/integration/select_readview_test.lua @@ -2498,7 +2498,11 @@ local function read_impl(cg, space, conditions, opts) rv:close() - return crud.unflatten_rows(resp.rows, resp.metadata) + if err ~= nil then + return nil, err + end + + return crud.unflatten_rows(resp.rows, resp.metadata), nil end, {space, conditions, opts}) end @@ -2518,3 +2522,17 @@ for case_name_template, case in pairs(gh_373_types_cases) do case(g, read_impl) end end + +pgroup.before_test( + 'test_select_merger_process_storage_error', + read_scenario.before_merger_process_storage_error +) + +pgroup.test_select_merger_process_storage_error = function(g) + read_scenario.merger_process_storage_error(g, read_impl) +end + +pgroup.after_test( + 'test_select_merger_process_storage_error', + read_scenario.after_merger_process_storage_error +) diff --git a/test/integration/select_test.lua b/test/integration/select_test.lua index 0d546704..95ded061 100644 --- a/test/integration/select_test.lua +++ b/test/integration/select_test.lua @@ -2274,9 +2274,12 @@ local function read_impl(cg, space, conditions, opts) opts.mode = 'write' local resp, err = cg.cluster.main_server:call('crud.select', {space, conditions, opts}) - t.assert_equals(err, nil) - return crud.unflatten_rows(resp.rows, resp.metadata) + if err ~= nil then + return nil, err + end + + return crud.unflatten_rows(resp.rows, resp.metadata), nil end pgroup.test_gh_418_select_with_secondary_noneq_index_condition = function(g) @@ -2295,3 +2298,17 @@ for case_name_template, case in pairs(gh_373_types_cases) do case(g, read_impl) end end + +pgroup.before_test( + 'test_select_merger_process_storage_error', + read_scenario.before_merger_process_storage_error +) + +pgroup.test_select_merger_process_storage_error = function(g) + read_scenario.merger_process_storage_error(g, read_impl) +end + +pgroup.after_test( + 'test_select_merger_process_storage_error', + read_scenario.after_merger_process_storage_error +)