From d0dc5c7c041162ce338510f1a73d16deccba2084 Mon Sep 17 00:00:00 2001 From: Alexander Turenko Date: Thu, 5 Jul 2018 16:31:16 +0300 Subject: [PATCH 1/2] Handle unique index constraint violation in shard Fixes #188. --- graphql/accessor_shard.lua | 104 ++++++++++++++++++---- test/common/introspection.test.lua | 136 +++++++++++++++++++++-------- test/common/mutation.test.lua | 102 +++++++++++++++++++--- test/test_utils.lua | 8 ++ test/testdata/common_testdata.lua | 19 +++- 5 files changed, 306 insertions(+), 63 deletions(-) diff --git a/graphql/accessor_shard.lua b/graphql/accessor_shard.lua index bfb638e..5b67675 100644 --- a/graphql/accessor_shard.lua +++ b/graphql/accessor_shard.lua @@ -4,6 +4,7 @@ local json = require('json') local yaml = require('yaml') +local digest = require('digest') local utils = require('graphql.utils') local shard = utils.optional_require('shard') local accessor_general = require('graphql.accessor_general') @@ -22,6 +23,18 @@ local index_info_cache = {} local function shard_check_error(func_name, result, err) if result ~= nil then return end + + -- avoid json encoding of an error message (when the error is in the known + -- format) + if type(err) == 'table' and type(err.error) == 'string' then + error({ + message = err.error, + extensions = { + shard_error = err, + } + }) + end + error(('%s: %s'):format(func_name, json.encode(err))) end @@ -215,6 +228,12 @@ local function space_operation(collection_name, nodes, operation, ...) return master_result end +local function get_shard_key_hash(key) + local shards_n = #shard.shards + local num = type(key) == 'number' and key or digest.crc32(key) + return 1 + digest.guava(num, shards_n) +end + -- }}} --- Check whether a collection (it is sharded space for that accessor) exists. @@ -395,6 +414,46 @@ end --- Update a tuple with an update statements. --- +--- In case when the update should change the storage where the tuple stored +--- we perform insert to the new storage and delete from the old one. The +--- order must be 'first insert, then delete', because insert can report an +--- error in case of unique index constraints violation and we must not +--- perform delete in the case. +--- +--- This function emulates (more or less preciselly, see below) behaviour of +--- update as if it would be performed on a local tarantool instance. In case +--- when the tuple resides on the same storage the update operation performs +--- a unique index constraints check within the storage, but not on the overall +--- cluster. In case when the tuple changes its storage the insert operation +--- performs the check within the target storage. +--- +--- We can consider this as relaxing of the constraints: the function can +--- silently violate cluster-wide uniqueness constraints or report a +--- violation that was introduced by some previous operation, but cannot +--- report a violation when a local tarantool space would not. +--- +--- 'Insert, then delete' approach is applicable and do not lead to a false +--- positive unique index constraint violation when storage nodes are different +--- and do not contain same tuples. We check the first condition in the +--- function and the second is guaranteed by the shard module. +--- +--- Note: if one want to use this function as basis for a similar one, but +--- allowing update of a primary key the following details should be noticed. A +--- primary key update that **changes a storage** where the tuple saved can be +--- performed with the 'insert, then delete' approach. An update **within one +--- storage** cannot be performed in the following ways: +--- +--- * as update (because tarantool forbids update of a primary key), +--- * 'insert, then delete' way (because insert can report a unique index +--- constraint violation due to values in the old version of the tuple), +--- * 'tuple:update(), then replace' (at least because old tuple resides in the +--- storage and because an other tuple can be silently rewritten). +--- +--- To support primary key update for **one storage** case one can use 'delete, +--- then insert' way and perform the rollback action (insert old tuple) in case +--- when insert of the new tuple reports an error. There are other ways, e.g. +--- manual unique constraints check. +--- --- @tparam table self accessor_general instance --- --- @tparam string collection_name @@ -420,16 +479,6 @@ local function update_tuple(self, collection_name, key, statements, opts) shard_check_status(func_name) - local is_shard_key_to_be_updated = false - for _, statement in ipairs(statements) do - -- statement is {operator, field_no, value} - local field_no = statement[2] - if field_no == SHARD_KEY_FIELD_NO then - is_shard_key_to_be_updated = true - break - end - end - -- We follow tarantool convention and disallow update of primary key parts. local primary_index_info = get_index_info(collection_name, 0) for _, statement in ipairs(statements) do @@ -443,14 +492,37 @@ local function update_tuple(self, collection_name, key, statements, opts) end end + local is_shard_key_to_be_updated = false + local new_shard_key_value + for _, statement in ipairs(statements) do + -- statement is {operator, field_no, value} + local field_no = statement[2] + if field_no == SHARD_KEY_FIELD_NO then + is_shard_key_to_be_updated = true + new_shard_key_value = statement[3] + break + end + end + + local tuple = opts.tuple or get_tuple(self, collection_name, key) + + local is_storage_to_be_changed = false if is_shard_key_to_be_updated then - local tuple = self.funcs.delete_tuple(self, collection_name, key, - {tuple = opts.tuple}) - tuple = tuple:update(statements) - return self.funcs.insert_tuple(self, collection_name, tuple) + local old_shard_key_value = tuple[1] + local old_shard_key_hash = get_shard_key_hash(old_shard_key_value) + local new_shard_key_hash = get_shard_key_hash(new_shard_key_value) + is_storage_to_be_changed = old_shard_key_hash ~= new_shard_key_hash + end + + if is_storage_to_be_changed then + -- different storages case + local old_tuple = opts.tuple or get_tuple(self, collection_name, key) + local new_tuple = old_tuple:update(statements) + self.funcs.insert_tuple(self, collection_name, new_tuple) + self.funcs.delete_tuple(self, collection_name, key, {tuple = old_tuple}) + return new_tuple else - local tuple = opts.tuple or get_tuple(self, collection_name, - key) + -- one storage case local nodes = shard.shard(tuple[SHARD_KEY_FIELD_NO]) local tuple = space_operation(collection_name, nodes, 'update', key, statements) diff --git a/test/common/introspection.test.lua b/test/common/introspection.test.lua index dea6ce0..35102c3 100755 --- a/test/common/introspection.test.lua +++ b/test/common/introspection.test.lua @@ -347,6 +347,10 @@ local function run_queries(gql_wrapper) name: Int kind: SCALAR name: limit + - type: + name: String + kind: SCALAR + name: order_metainfo_id_copy - type: name: String kind: SCALAR @@ -789,6 +793,14 @@ local function run_queries(gql_wrapper) kind: SCALAR kind: NON_NULL name: order_metainfo_id + - isDeprecated: false + args: *0 + type: + ofType: + name: String + kind: SCALAR + kind: NON_NULL + name: order_metainfo_id_copy - isDeprecated: false args: *0 type: @@ -857,6 +869,12 @@ local function run_queries(gql_wrapper) kind: SCALAR kind: NON_NULL name: order_metainfo_id + - type: + ofType: + name: String + kind: SCALAR + kind: NON_NULL + name: order_metainfo_id_copy - type: ofType: name: String @@ -1029,14 +1047,18 @@ local function run_queries(gql_wrapper) name: String kind: SCALAR name: offset + - type: + name: Int + kind: SCALAR + name: limit - type: name: String kind: SCALAR name: order_metainfo_id - type: - name: Int + name: String kind: SCALAR - name: limit + name: order_metainfo_id_copy - type: name: String kind: SCALAR @@ -1436,6 +1458,10 @@ local function run_queries(gql_wrapper) name: String kind: SCALAR name: order_metainfo_id + - type: + name: String + kind: SCALAR + name: order_metainfo_id_copy - type: name: String kind: SCALAR @@ -1453,21 +1479,25 @@ local function run_queries(gql_wrapper) - kind: INPUT_OBJECT inputFields: - type: - name: arguments___order_metainfo_collection___store___store - kind: INPUT_OBJECT - name: store + name: String + kind: SCALAR + name: order_metainfo_id - type: name: String kind: SCALAR - name: metainfo + name: order_metainfo_id_copy - type: name: String kind: SCALAR - name: order_id + name: metainfo - type: name: String kind: SCALAR - name: order_metainfo_id + name: order_id + - type: + name: arguments___order_metainfo_collection___store___store + kind: INPUT_OBJECT + name: store name: order_metainfo_connection description: generated from the connection "order_metainfo_connection" of collection "order_collection" using collection "order_metainfo_collection" @@ -1552,6 +1582,10 @@ local function run_queries(gql_wrapper) name: Boolean kind: SCALAR name: delete + - type: + name: String + kind: SCALAR + name: metainfo - type: name: String kind: SCALAR @@ -1559,7 +1593,7 @@ local function run_queries(gql_wrapper) - type: name: String kind: SCALAR - name: metainfo + name: order_metainfo_id_copy - type: name: String kind: SCALAR @@ -1692,6 +1726,10 @@ local function run_queries(gql_wrapper) same among all values type - kind: INPUT_OBJECT inputFields: + - type: + name: String + kind: SCALAR + name: order_metainfo_id_copy - type: name: String kind: SCALAR @@ -2037,14 +2075,18 @@ local function run_queries(gql_wrapper) name: String kind: SCALAR name: offset + - type: + name: Int + kind: SCALAR + name: limit - type: name: String kind: SCALAR name: order_metainfo_id - type: - name: Int + name: String kind: SCALAR - name: limit + name: order_metainfo_id_copy - type: name: String kind: SCALAR @@ -2275,21 +2317,25 @@ local function run_queries(gql_wrapper) - kind: INPUT_OBJECT inputFields: - type: - name: arguments___order_metainfo_collection___store___store - kind: INPUT_OBJECT - name: store + name: String + kind: SCALAR + name: order_metainfo_id - type: name: String kind: SCALAR - name: metainfo + name: order_metainfo_id_copy - type: name: String kind: SCALAR - name: order_id + name: metainfo - type: name: String kind: SCALAR - name: order_metainfo_id + name: order_id + - type: + name: arguments___order_metainfo_collection___store___store + kind: INPUT_OBJECT + name: store name: order_metainfo_connection description: generated from the connection "order_metainfo_connection" of collection "order_collection" using collection "order_metainfo_collection" @@ -2438,14 +2484,18 @@ local function run_queries(gql_wrapper) name: String kind: SCALAR name: offset + - type: + name: Int + kind: SCALAR + name: limit - type: name: String kind: SCALAR name: order_metainfo_id - type: - name: Int + name: String kind: SCALAR - name: limit + name: order_metainfo_id_copy - type: name: String kind: SCALAR @@ -2610,9 +2660,9 @@ local function run_queries(gql_wrapper) description: A list of all directives supported by this server. kind: OBJECT name: __Schema - description: A GraphQL Schema defines the capabilities of a GraphQL server. It - exposes all available types and directives on the server, as well as the entry - points for query and mutation operations. + description: A GraphQL Schema defines the capabilities of a GraphQL server. + It exposes all available types and directives on the server, as well as the + entry points for query and mutation operations. - interfaces: *0 fields: - isDeprecated: false @@ -2646,8 +2696,8 @@ local function run_queries(gql_wrapper) kind: OBJECT name: __EnumValue description: One possible value for a given Enum. Enum values are unique values, - not a placeholder for a string or numeric value. However an Enum value is returned - in a JSON response as a string. + not a placeholder for a string or numeric value. However an Enum value is + returned in a JSON response as a string. - interfaces: *0 fields: - isDeprecated: false @@ -2742,15 +2792,19 @@ local function run_queries(gql_wrapper) many kinds of types in GraphQL as represented by the `__TypeKind` enum. Depending on the kind of a type, certain fields describe information about that type. Scalar types provide no information beyond a name and description, while Enum - types provide their values. Object and Interface types provide the fields they - describe. Abstract types, Union and Interface, provide the Object types possible - at runtime. List and NonNull types compose other types. + types provide their values. Object and Interface types provide the fields + they describe. Abstract types, Union and Interface, provide the Object types + possible at runtime. List and NonNull types compose other types. - kind: INPUT_OBJECT inputFields: - type: name: String kind: SCALAR name: order_metainfo_id + - type: + name: String + kind: SCALAR + name: order_metainfo_id_copy - type: name: String kind: SCALAR @@ -2795,8 +2849,8 @@ local function run_queries(gql_wrapper) name: String kind: SCALAR name: defaultValue - description: A GraphQL-formatted string representing the default value for this - input value. + description: A GraphQL-formatted string representing the default value for + this input value. kind: OBJECT name: __InputValue description: Arguments provided to Fields or Directives and the input fields @@ -2880,6 +2934,14 @@ local function run_queries(gql_wrapper) kind: SCALAR kind: NON_NULL name: order_metainfo_id + - isDeprecated: false + args: *0 + type: + ofType: + name: String + kind: SCALAR + kind: NON_NULL + name: order_metainfo_id_copy - isDeprecated: false args: *0 type: @@ -2965,21 +3027,25 @@ local function run_queries(gql_wrapper) - isDeprecated: false args: - type: - name: arguments___order_metainfo_collection___store___store - kind: INPUT_OBJECT - name: store + name: String + kind: SCALAR + name: order_metainfo_id - type: name: String kind: SCALAR - name: metainfo + name: order_metainfo_id_copy - type: name: String kind: SCALAR - name: order_id + name: metainfo - type: name: String kind: SCALAR - name: order_metainfo_id + name: order_id + - type: + name: arguments___order_metainfo_collection___store___store + kind: INPUT_OBJECT + name: store type: name: order_metainfo_collection kind: OBJECT diff --git a/test/common/mutation.test.lua b/test/common/mutation.test.lua index d218e21..b672e47 100755 --- a/test/common/mutation.test.lua +++ b/test/common/mutation.test.lua @@ -11,6 +11,8 @@ local yaml = require('yaml') local utils = require('graphql.utils') local test_utils = require('test.test_utils') local testdata = require('test.testdata.common_testdata') +local test_run = utils.optional_require('test_run') +test_run = test_run and test_run.new() box.cfg({}) @@ -145,6 +147,7 @@ local function check_insert_order_metainfo(test, gql_wrapper, virtbox, local exp_tuple = { 'order metainfo 4000', 'order_metainfo_id_4000', + 'order_metainfo_id_4000', 'order_id_4000', 'store 4000', 'street 4000', @@ -269,10 +272,10 @@ local function check_update_order_metainfo(test, gql_wrapper, virtbox, local orig_tuple = get_tuple(virtbox, 'order_metainfo_collection', {order_metainfo_id}) local exp_orig_tuple = { - 'order metainfo 1', order_metainfo_id, 'order_id_1', 'store 1', - 'street 1', 'city 1', 'state 1', 'zip 1', 'second street 1', - 'second city 1', 'second state 1', 'second zip 1', - EXTERNAL_ID_INT, 1, {'fast', 'new'}, { + 'order metainfo 1', order_metainfo_id, order_metainfo_id, + 'order_id_1', 'store 1', 'street 1', 'city 1', 'state 1', + 'zip 1', 'second street 1', 'second city 1', 'second state 1', + 'second zip 1', EXTERNAL_ID_INT, 1, {'fast', 'new'}, { size = 'medium', since = '2018-01-01', }, @@ -291,12 +294,12 @@ local function check_update_order_metainfo(test, gql_wrapper, virtbox, test:ok(tuple ~= nil, 'updated tuple exists') local exp_tuple = table.copy(exp_orig_tuple) exp_tuple[1] = 'changed' - exp_tuple[6] = 'changed city' - exp_tuple[10] = 'second changed city' - exp_tuple[13] = EXTERNAL_ID_STRING - exp_tuple[14] = 'eid changed' - exp_tuple[15] = {'slow'} - exp_tuple[16] = {size = 'small'} + exp_tuple[7] = 'changed city' + exp_tuple[11] = 'second changed city' + exp_tuple[14] = EXTERNAL_ID_STRING + exp_tuple[15] = 'eid changed' + exp_tuple[16] = {'slow'} + exp_tuple[17] = {size = 'small'} test:is_deeply(tuple:totable(), exp_tuple, 'updated tuple is correct') -- replace back updated tuples & check @@ -353,9 +356,13 @@ local function check_delete(test, gql_wrapper, virtbox, mutation_delete, end) end +local function extract_storage_error(err) + return err:gsub('^failed to execute operation on[^:]+: *', '') +end + local function run_queries(gql_wrapper, virtbox, meta) local test = tap.test('mutation') - test:plan(22) + test:plan(23) -- {{{ insert @@ -524,6 +531,7 @@ local function run_queries(gql_wrapper, virtbox, meta) order_metainfo_collection(insert: { metainfo: "order metainfo 4000" order_metainfo_id: "order_metainfo_id_4000" + order_metainfo_id_copy: "order_metainfo_id_4000" order_id: "order_id_4000" store: { name: "store 4000" @@ -548,6 +556,7 @@ local function run_queries(gql_wrapper, virtbox, meta) }) { metainfo order_metainfo_id + order_metainfo_id_copy order_id store { name @@ -577,6 +586,7 @@ local function run_queries(gql_wrapper, virtbox, meta) order_metainfo_collection: - metainfo: order metainfo 4000 order_metainfo_id: order_metainfo_id_4000 + order_metainfo_id_copy: order_metainfo_id_4000 order_id: order_id_4000 store: name: store 4000 @@ -604,6 +614,7 @@ local function run_queries(gql_wrapper, virtbox, meta) order_metainfo_collection(insert: $order_metainfo) { metainfo order_metainfo_id + order_metainfo_id_copy order_id store { name @@ -635,6 +646,7 @@ local function run_queries(gql_wrapper, virtbox, meta) order_metainfo = { metainfo = 'order metainfo 4000', order_metainfo_id = 'order_metainfo_id_4000', + order_metainfo_id_copy = 'order_metainfo_id_4000', order_id = 'order_id_4000', store = { name = 'store 4000', @@ -1167,6 +1179,74 @@ local function run_queries(gql_wrapper, virtbox, meta) test:is(err, exp_err, 'updating of a field of a primary key when it is shard key field') + -- violation of an unique index constraint + local mutation_update_6 = [[ + mutation { + order_metainfo_collection( + order_metainfo_id: "order_metainfo_id_1" + update: { + metainfo: "updated" + order_metainfo_id_copy: "order_metainfo_id_2" + } + ) { + metainfo + order_metainfo_id + order_metainfo_id_copy + order_id + store { + name + } + } + } + ]] + local conf_name = test_run and test_run:get_cfg('conf') or 'space' + if conf_name:startswith('shard') then + local old_shard_key_hash = test_utils.get_shard_key_hash( + 'order_metainfo_id_1') + local new_shard_key_hash = test_utils.get_shard_key_hash( + 'order_metainfo_id_2') + -- check the case is really involving moving a tuple from one storage + -- to an another + assert(old_shard_key_hash ~= new_shard_key_hash) + end + local gql_mutation_update_6 = gql_wrapper:compile(mutation_update_6) + test:test('unique constraint violation', function(test) + test_utils.show_trace(function() + test:plan(4) + local order_metainfo_id = 'order_metainfo_id_1' + + -- save the original tuple + local orig_tuple = get_tuple(virtbox, 'order_metainfo_collection', + {order_metainfo_id}) + + -- check mutation result from graphql + local result = gql_mutation_update_6:execute({}) + local exp_err = "Duplicate key exists in unique index " .. + "'order_metainfo_id_copy_index' in space " .. + "'order_metainfo_collection'" + local err = extract_storage_error(result.errors[1].message) + test:is(err, exp_err, 'update result') + + -- check the user was not changed + local tuple = get_tuple(virtbox, 'order_metainfo_collection', + {order_metainfo_id}) + test:ok(tuple ~= nil, 'updated tuple exists') + test:is_deeply((tuple or box.tuple.new({})):totable(), + orig_tuple:totable(), 'tuple was not changed') + + -- replace back the tuple & check (in case the test fails and it + -- was updated) + replace_tuple(virtbox, 'order_metainfo_collection', + {order_metainfo_id}, orig_tuple) + local tuple = get_tuple(virtbox, 'order_metainfo_collection', + {order_metainfo_id}) + test:is_deeply(tuple:totable(), orig_tuple:totable(), + 'tuple was replaced back') + + -- test:check() will be called automatically by the tap module + end) + end) + -- }}} -- {{{ delete diff --git a/test/test_utils.lua b/test/test_utils.lua index ccb56dc..46a60cb 100644 --- a/test/test_utils.lua +++ b/test/test_utils.lua @@ -8,6 +8,8 @@ package.path = fio.abspath(debug.getinfo(1).source:match("@?(.*/)") local log = require('log') local avro_schema = require('avro_schema') +local digest = require('digest') +local shard = require('shard') local graphql = require('graphql') local multirunner = require('test.common.multirunner') local utils = require('graphql.utils') @@ -179,4 +181,10 @@ function test_utils.deeply_number_tostring(t) end end +function test_utils.get_shard_key_hash(key) + local shards_n = #shard.shards + local num = type(key) == 'number' and key or digest.crc32(key) + return 1 + digest.guava(num, shards_n) +end + return test_utils diff --git a/test/testdata/common_testdata.lua b/test/testdata/common_testdata.lua index 89b7b85..5809d21 100644 --- a/test/testdata/common_testdata.lua +++ b/test/testdata/common_testdata.lua @@ -42,6 +42,7 @@ function common_testdata.get_test_metadata() "fields": [ { "name": "metainfo", "type": "string" }, { "name": "order_metainfo_id", "type": "string" }, + { "name": "order_metainfo_id_copy", "type": "string" }, { "name": "order_id", "type": "string" }, { "name": "store", "type": { "type": "record", @@ -163,6 +164,13 @@ function common_testdata.get_test_metadata() unique = true, primary = true, }, + order_metainfo_id_copy_index = { + service_fields = {}, + fields = {'order_metainfo_id_copy'}, + index_type = 'tree', + unique = true, + primary = false, + }, order_id_index = { service_fields = {}, fields = {'order_id'}, @@ -191,7 +199,8 @@ function common_testdata.init_spaces() -- order_metainfo_collection fields local M_ORDER_METAINFO_ID_FN = 2 - local M_ORDER_ID_FN = 3 + local M_ORDER_METAINFO_ID_COPY_FN = 3 + local M_ORDER_ID_FN = 4 box.once('test_space_init_spaces', function() box.schema.create_space('user_collection') @@ -218,6 +227,12 @@ function common_testdata.init_spaces() M_ORDER_METAINFO_ID_FN, 'string' }} ) + box.space.order_metainfo_collection:create_index( + 'order_metainfo_id_copy_index', + {type = 'tree', parts = { + M_ORDER_METAINFO_ID_COPY_FN, 'string' + }} + ) box.space.order_metainfo_collection:create_index('order_id_index', {type = 'tree', parts = { M_ORDER_ID_FN, 'string' @@ -314,6 +329,7 @@ function common_testdata.fill_test_data(virtbox, meta) test_utils.replace_object(virtbox, meta, 'order_metainfo_collection', { metainfo = 'order metainfo ' .. s, order_metainfo_id = 'order_metainfo_id_' .. s, + order_metainfo_id_copy = 'order_metainfo_id_' .. s, order_id = 'order_id_' .. s, store = { name = 'store ' .. s, @@ -1520,6 +1536,7 @@ type_mismatch_cases = function(gql_wrapper, test) order_metainfo = { metainfo = 'order metainfo', order_metainfo_id = 'order_metainfo_id_14_1', + order_metainfo_id_copy = 'order_metainfo_id_14_1', order_id = 'order_id', store = { name = 'store', From b5ea0d2f139d6237d7f2a52b4b304f2a758b37fb Mon Sep 17 00:00:00 2001 From: Alexander Turenko Date: Fri, 6 Jul 2018 16:56:51 +0300 Subject: [PATCH 2/2] Tiny fix in the comment to TIMEOUT_INFINITY --- graphql/accessor_general.lua | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/graphql/accessor_general.lua b/graphql/accessor_general.lua index 42a4ddd..ecdf79d 100644 --- a/graphql/accessor_general.lua +++ b/graphql/accessor_general.lua @@ -35,8 +35,8 @@ local DEF_TIMEOUT_MS = 1000 -- save start time at start, calculate current time on each iteration and -- substract the start time from it, compare with the timeout. With such -- approch we don't add the timeout in nanoseconds to a start time and can --- remove the divide by two below. -local TIMEOUT_INFINITY = 18446744073709551615ULL / (2 * 10^6) -- microseconds +-- remove the divide by two below. The value is roughly equal to 292 years. +local TIMEOUT_INFINITY = 18446744073709551615ULL / (2 * 10^6) -- milliseconds accessor_general.TIMEOUT_INFINITY = TIMEOUT_INFINITY