From 82a0518d52df3c4f6ecd667d10818b680cc0774e Mon Sep 17 00:00:00 2001 From: Alexander Turenko Date: Tue, 19 Oct 2021 04:22:16 +0300 Subject: [PATCH] Fix merger support recognition on tarantool 2.10+ We cannot use just string comparison as before, because '2.10.0-<...>' less than, say, '2.3' lexicographically. The module had to use the old select implementation on tarantool 2.10 due to this problem. Implemented more accurate checks, whether built-in and external tuple-merger are supported. Say, it'll not enable the built-in merger on tarantool 2.4.1, where the module has a known problem (see comments in the code). Fixed the problem of the same kind in a test case. When I replaced `_G._TARANTOOL` with just `_TARANTOOL`, the luacheck linter said me: | <...>/test/helper.lua:307:31: accessing undefined field match of | global _TARANTOOL On the line: | local major_minor_patch = _TARANTOOL:split('-', 1)[1] But when I added '_TARANTOOL' into globals, it stops to complain. Strange, but okay. I prefer just `_TARANTOOL` without `_G`. --- .luacheckrc | 2 +- CHANGELOG.md | 3 ++ crud/common/utils.lua | 45 +++++++++++++++++++++ crud/select.lua | 17 +++----- test/helper.lua | 23 +++++++++++ test/integration/simple_operations_test.lua | 4 +- 6 files changed, 80 insertions(+), 14 deletions(-) diff --git a/.luacheckrc b/.luacheckrc index 66f90ee3..79021dfe 100644 --- a/.luacheckrc +++ b/.luacheckrc @@ -1,5 +1,5 @@ redefined = false -globals = {'box', 'utf8', 'checkers'} +globals = {'box', 'utf8', 'checkers', '_TARANTOOL'} include_files = {'**/*.lua', '*.luacheckrc', '*.rockspec'} exclude_files = {'**/*.rocks/', 'tmp/', 'tarantool-enterprise/'} max_line_length = 120 diff --git a/CHANGELOG.md b/CHANGELOG.md index f3511d8c..f781e309 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed +* Use tuple-merger backed select implementation on tarantool 2.10+ (it gives + less pressure on Lua GC). + ## [0.9.0] - 20-10-21 ### Added diff --git a/crud/common/utils.lua b/crud/common/utils.lua index a42fbc29..ac3688aa 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -210,6 +210,35 @@ local function determine_enabled_features() -- since Tarantool 2.6.3 / 2.7.2 / 2.8.1 enabled_tarantool_features.jsonpath_indexes = major >= 3 or (major >= 2 and ((minor >= 6 and patch >= 3) or (minor >= 7 and patch >= 2) or (minor >= 8 and patch >= 1) or minor >= 9)) + + -- The merger module was implemented in 2.2.1, see [1]. + -- However it had the critical problem [2], which leads to + -- segfault at attempt to use the module from a fiber serving + -- iproto request. So we don't use it in versions before the + -- fix. + -- + -- [1]: https://github.com/tarantool/tarantool/issues/3276 + -- [2]: https://github.com/tarantool/tarantool/issues/4954 + enabled_tarantool_features.builtin_merger = + (major == 2 and minor == 3 and patch >= 3) or + (major == 2 and minor == 4 and patch >= 2) or + (major == 2 and minor == 5 and patch >= 1) or + (major >= 2 and minor >= 6) or + (major >= 3) + + -- The external merger module leans on a set of relatively + -- new APIs in tarantool. So it works only on tarantool + -- versions, which offer those APIs. + -- + -- See README of the module: + -- https://github.com/tarantool/tuple-merger + enabled_tarantool_features.external_merger = + (major == 1 and minor == 10 and patch >= 8) or + (major == 2 and minor == 4 and patch >= 3) or + (major == 2 and minor == 5 and patch >= 2) or + (major == 2 and minor == 6 and patch >= 1) or + (major == 2 and minor >= 7) or + (major >= 3) end function utils.tarantool_supports_fieldpaths() @@ -236,6 +265,22 @@ function utils.tarantool_supports_jsonpath_indexes() return enabled_tarantool_features.jsonpath_indexes end +function utils.tarantool_has_builtin_merger() + if enabled_tarantool_features.builtin_merger == nil then + determine_enabled_features() + end + + return enabled_tarantool_features.builtin_merger +end + +function utils.tarantool_supports_external_merger() + if enabled_tarantool_features.external_merger == nil then + determine_enabled_features() + end + + return enabled_tarantool_features.external_merger +end + local function add_nullable_fields_recursive(operations, operations_map, space_format, tuple, id) if id < 2 or tuple[id - 1] ~= box.NULL then return operations diff --git a/crud/select.lua b/crud/select.lua index 41964e1f..2e9010d4 100644 --- a/crud/select.lua +++ b/crud/select.lua @@ -1,5 +1,6 @@ local errors = require('errors') +local utils = require('crud.common.utils') local select_executor = require('crud.select.executor') local select_filters = require('crud.select.filters') local dev_checks = require('crud.common.dev_checks') @@ -9,18 +10,12 @@ local SelectError = errors.new_class('SelectError') local select_module -if '2' <= _TARANTOOL and _TARANTOOL <= '2.3.3' then - -- "merger" segfaults here - -- See https://github.com/tarantool/tarantool/issues/4954 - select_module = require('crud.select.compat.select_old') -elseif not package.search('tuple.merger') and package.loaded['merger'] == nil then - -- we don't use pcall(require, modile_name) here because it - -- leads to ignoring errors other than 'No LuaRocks module found' - - -- "merger" isn't supported here - select_module = require('crud.select.compat.select_old') -else +local has_merger = (utils.tarantool_supports_external_merger() and + package.search('tuple.merger')) or utils.tarantool_has_builtin_merger() +if has_merger then select_module = require('crud.select.compat.select') +else + select_module = require('crud.select.compat.select_old') end local function make_cursor(data) diff --git a/test/helper.lua b/test/helper.lua index 58d46fc8..64f6b48f 100644 --- a/test/helper.lua +++ b/test/helper.lua @@ -300,4 +300,27 @@ function helpers.get_other_storage_bucket_id(cluster, bucket_id) ]], {bucket_id}) end +function helpers.tarantool_version_at_least(wanted_major, wanted_minor, + wanted_patch) + -- Borrowed from `determine_enabled_features()` from + -- crud/common/utils.lua. + local major_minor_patch = _TARANTOOL:split('-', 1)[1] + local major_minor_patch_parts = major_minor_patch:split('.', 2) + + local major = tonumber(major_minor_patch_parts[1]) + local minor = tonumber(major_minor_patch_parts[2]) + local patch = tonumber(major_minor_patch_parts[3]) + + if major < (wanted_major or 0) then return false end + if major > (wanted_major or 0) then return true end + + if minor < (wanted_minor or 0) then return false end + if minor > (wanted_minor or 0) then return true end + + if patch < (wanted_patch or 0) then return false end + if patch > (wanted_patch or 0) then return true end + + return true +end + return helpers diff --git a/test/integration/simple_operations_test.lua b/test/integration/simple_operations_test.lua index 8c33daa2..9ab7196d 100644 --- a/test/integration/simple_operations_test.lua +++ b/test/integration/simple_operations_test.lua @@ -432,11 +432,11 @@ pgroup.test_intermediate_nullable_fields_update = function(g) -- However since 2.8 Tarantool could update intermediate nullable fields -- (https://github.com/tarantool/tarantool/issues/3378). -- So before 2.8 update returns an error but after it update is correct. - if _TARANTOOL > "2.8" then + if helpers.tarantool_version_at_least(2, 8) then local _, err = g.cluster.main_server.net_box:call('crud.update', {'developers', 1, {{'=', '[5].a.b[1]', 3}, {'=', 'extra_5', 'extra_value_5'}}}) t.assert_equals(err, nil) - elseif _TARANTOOL >= "2.3" then + elseif helpers.tarantool_version_at_least(2, 3) then local _, err = g.cluster.main_server.net_box:call('crud.update', {'developers', 1, {{'=', '[5].a.b[1]', 3}, {'=', 'extra_5', 'extra_value_5'}}}) t.assert_equals(err.err, "Failed to update: Field ''extra_5'' was not found in the tuple")