From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 9213B24D3A for ; Wed, 18 Jul 2018 05:24:18 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id AeTwAdrp9rdY for ; Wed, 18 Jul 2018 05:24:18 -0400 (EDT) Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1DFE4241BA for ; Wed, 18 Jul 2018 05:24:18 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 4/7] box: bove some decode functions from alter.cc References: <0a12bea63784d9d44744d4d49e1ac9b2f7c0d3ea.1531903378.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <404a66f4-b93f-4f24-3378-eac9b584c540@tarantool.org> Date: Wed, 18 Jul 2018 12:24:15 +0300 MIME-Version: 1.0 In-Reply-To: <0a12bea63784d9d44744d4d49e1ac9b2f7c0d3ea.1531903378.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, imeevma@tarantool.org, Kirill Shcherbatov Kirill, please, do a second review. Mergen, try to avoid creating new thread on each commit. Either send responses using --in-reply-to, or send all the emails together under v2 in a new thread. On 18/07/2018 11:43, imeevma@tarantool.org wrote: > Move functions to decode space format and > functions to decode index options from > alter.cc to format_def.c and index_def.c. > > Part of #3375. > --- > Branch: https://github.com/tarantool/tarantool/compare/imeevma/gh-3375-lua-expose-ephemeral-spaces > Issue: https://github.com/tarantool/tarantool/issues/3375 > > src/box/alter.cc | 215 ++-------------------------------------------------- > src/box/index_def.c | 60 +++++++++++++++ > src/box/index_def.h | 8 ++ > src/box/space_def.c | 155 +++++++++++++++++++++++++++++++++++++ > src/box/space_def.h | 18 +++++ > 5 files changed, 246 insertions(+), 210 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 7b6bd1a..47729d7 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -30,27 +30,18 @@ > */ > #include "alter.h" > #include "schema.h" > -#include "user.h" > -#include "space.h" > -#include "index.h" > #include "func.h" > #include "coll_id_cache.h" > #include "coll_id_def.h" > #include "txn.h" > #include "tuple.h" > -#include "fiber.h" /* for gc_pool */ > #include "scoped_guard.h" > #include "third_party/base64.h" > -#include /* for placement new */ > -#include /* snprintf() */ > -#include > #include "replication.h" /* for replica_set_id() */ > #include "session.h" /* to fetch the current user. */ > -#include "vclock.h" /* VCLOCK_MAX */ > #include "xrow.h" > #include "iproto_constants.h" > #include "identifier.h" > -#include "version.h" > #include "sequence.h" > #include "sql.h" > > @@ -167,61 +158,6 @@ err: > } > > /** > - * Fill index_opts structure from opts field in tuple of space _index > - * Throw an error is unrecognized option. > - */ > -static void > -index_opts_decode(struct index_opts *opts, const char *map, > - struct region *region) > -{ > - index_opts_create(opts); > - if (opts_decode(opts, index_opts_reg, &map, ER_WRONG_INDEX_OPTIONS, > - BOX_INDEX_FIELD_OPTS, region) != 0) > - diag_raise(); > - if (opts->distance == rtree_index_distance_type_MAX) { > - tnt_raise(ClientError, ER_WRONG_INDEX_OPTIONS, > - BOX_INDEX_FIELD_OPTS, "distance must be either "\ > - "'euclid' or 'manhattan'"); > - } > - if (opts->sql != NULL) { > - char *sql = strdup(opts->sql); > - if (sql == NULL) { > - opts->sql = NULL; > - tnt_raise(OutOfMemory, strlen(opts->sql) + 1, "strdup", > - "sql"); > - } > - opts->sql = sql; > - } > - if (opts->range_size <= 0) { > - tnt_raise(ClientError, ER_WRONG_INDEX_OPTIONS, > - BOX_INDEX_FIELD_OPTS, > - "range_size must be greater than 0"); > - } > - if (opts->page_size <= 0 || opts->page_size > opts->range_size) { > - tnt_raise(ClientError, ER_WRONG_INDEX_OPTIONS, > - BOX_INDEX_FIELD_OPTS, > - "page_size must be greater than 0 and " > - "less than or equal to range_size"); > - } > - if (opts->run_count_per_level <= 0) { > - tnt_raise(ClientError, ER_WRONG_INDEX_OPTIONS, > - BOX_INDEX_FIELD_OPTS, > - "run_count_per_level must be greater than 0"); > - } > - if (opts->run_size_ratio <= 1) { > - tnt_raise(ClientError, ER_WRONG_INDEX_OPTIONS, > - BOX_INDEX_FIELD_OPTS, > - "run_size_ratio must be greater than 1"); > - } > - if (opts->bloom_fpr <= 0 || opts->bloom_fpr > 1) { > - tnt_raise(ClientError, ER_WRONG_INDEX_OPTIONS, > - BOX_INDEX_FIELD_OPTS, > - "bloom_fpr must be greater than 0 and " > - "less than or equal to 1"); > - } > -} > - > -/** > * Create a index_def object from a record in _index > * system space. > * > @@ -250,7 +186,8 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space) > const char *opts_field = > tuple_field_with_type_xc(tuple, BOX_INDEX_FIELD_OPTS, > MP_MAP); > - index_opts_decode(&opts, opts_field, &fiber()->gc); > + if (index_opts_decode(&opts, opts_field, &fiber()->gc) != 0) > + diag_raise(); > const char *parts = tuple_field(tuple, BOX_INDEX_FIELD_PARTS); > uint32_t part_count = mp_decode_array(&parts); > if (name_len > BOX_NAME_MAX) { > @@ -316,149 +253,6 @@ space_opts_decode(struct space_opts *opts, const char *map, > } > > /** > - * Decode field definition from MessagePack map. Format: > - * {name: , type: }. Type is optional. > - * @param[out] field Field to decode to. > - * @param data MessagePack map to decode. > - * @param space_name Name of a space, from which the field is got. > - * Used in error messages. > - * @param name_len Length of @a space_name. > - * @param errcode Error code to use for client errors. Either > - * create or modify space errors. > - * @param fieldno Field number to decode. Used in error messages. > - * @param region Region to allocate field name. > - */ > -static void > -field_def_decode(struct field_def *field, const char **data, > - const char *space_name, uint32_t name_len, > - uint32_t errcode, uint32_t fieldno, struct region *region) > -{ > - if (mp_typeof(**data) != MP_MAP) { > - tnt_raise(ClientError, errcode, tt_cstr(space_name, name_len), > - tt_sprintf("field %d is not map", > - fieldno + TUPLE_INDEX_BASE)); > - } > - int count = mp_decode_map(data); > - *field = field_def_default; > - bool is_action_missing = true; > - uint32_t action_literal_len = strlen("nullable_action"); > - for (int i = 0; i < count; ++i) { > - if (mp_typeof(**data) != MP_STR) { > - tnt_raise(ClientError, errcode, > - tt_cstr(space_name, name_len), > - tt_sprintf("field %d format is not map"\ > - " with string keys", > - fieldno + TUPLE_INDEX_BASE)); > - } > - uint32_t key_len; > - const char *key = mp_decode_str(data, &key_len); > - if (opts_parse_key(field, field_def_reg, key, key_len, data, > - ER_WRONG_SPACE_FORMAT, > - fieldno + TUPLE_INDEX_BASE, region, > - true) != 0) > - diag_raise(); > - if (is_action_missing && > - key_len == action_literal_len && > - memcmp(key, "nullable_action", action_literal_len) == 0) > - is_action_missing = false; > - } > - if (is_action_missing) { > - field->nullable_action = field->is_nullable ? > - ON_CONFLICT_ACTION_NONE > - : ON_CONFLICT_ACTION_DEFAULT; > - } > - if (field->name == NULL) { > - tnt_raise(ClientError, errcode, tt_cstr(space_name, name_len), > - tt_sprintf("field %d name is not specified", > - fieldno + TUPLE_INDEX_BASE)); > - } > - size_t field_name_len = strlen(field->name); > - if (field_name_len > BOX_NAME_MAX) { > - tnt_raise(ClientError, errcode, tt_cstr(space_name, name_len), > - tt_sprintf("field %d name is too long", > - fieldno + TUPLE_INDEX_BASE)); > - } > - identifier_check_xc(field->name, field_name_len); > - if (field->type == field_type_MAX) { > - tnt_raise(ClientError, errcode, tt_cstr(space_name, name_len), > - tt_sprintf("field %d has unknown field type", > - fieldno + TUPLE_INDEX_BASE)); > - } > - if (field->nullable_action == on_conflict_action_MAX) { > - tnt_raise(ClientError, errcode, tt_cstr(space_name, name_len), > - tt_sprintf("field %d has unknown field on conflict " > - "nullable action", > - fieldno + TUPLE_INDEX_BASE)); > - } > - if (!((field->is_nullable && field->nullable_action == > - ON_CONFLICT_ACTION_NONE) > - || (!field->is_nullable > - && field->nullable_action != ON_CONFLICT_ACTION_NONE))) { > - tnt_raise(ClientError, errcode, tt_cstr(space_name, name_len), > - tt_sprintf("field %d has conflicting nullability and " > - "nullable action properties", fieldno + > - TUPLE_INDEX_BASE)); > - } > - if (field->coll_id != COLL_NONE && > - field->type != FIELD_TYPE_STRING && > - field->type != FIELD_TYPE_SCALAR && > - field->type != FIELD_TYPE_ANY) { > - tnt_raise(ClientError, errcode, tt_cstr(space_name, name_len), > - tt_sprintf("collation is reasonable only for " > - "string, scalar and any fields")); > - } > - > - const char *dv = field->default_value; > - if (dv != NULL) { > - field->default_value_expr = sql_expr_compile(sql_get(), dv, > - strlen(dv)); > - if (field->default_value_expr == NULL) > - diag_raise(); > - } > -} > - > -/** > - * Decode MessagePack array of fields. > - * @param data MessagePack array of fields. > - * @param[out] out_count Length of a result array. > - * @param space_name Space name to use in error messages. > - * @param errcode Errcode for client errors. > - * @param region Region to allocate result array. > - * > - * @retval Array of fields. > - */ > -static struct field_def * > -space_format_decode(const char *data, uint32_t *out_count, > - const char *space_name, uint32_t name_len, > - uint32_t errcode, struct region *region) > -{ > - /* Type is checked by _space format. */ > - assert(mp_typeof(*data) == MP_ARRAY); > - uint32_t count = mp_decode_array(&data); > - *out_count = count; > - if (count == 0) > - return NULL; > - size_t size = count * sizeof(struct field_def); > - struct field_def *region_defs = > - (struct field_def *) region_alloc_xc(region, size); > - /* > - * Nullify to prevent a case when decoding will fail in > - * the middle and space_def_destroy_fields() below will > - * work with garbage pointers. > - */ > - memset(region_defs, 0, size); > - auto fields_guard = make_scoped_guard([=] { > - space_def_destroy_fields(region_defs, count); > - }); > - for (uint32_t i = 0; i < count; ++i) { > - field_def_decode(®ion_defs[i], &data, space_name, name_len, > - errcode, i, region); > - } > - fields_guard.is_active = false; > - return region_defs; > -} > - > -/** > * Fill space_def structure from struct tuple. > */ > static struct space_def * > @@ -508,8 +302,9 @@ space_def_new_from_tuple(struct tuple *tuple, uint32_t errcode, > const char *format = > tuple_field_with_type_xc(tuple, BOX_SPACE_FIELD_FORMAT, > MP_ARRAY); > - fields = space_format_decode(format, &field_count, name, > - name_len, errcode, region); > + if (space_format_decode(&fields, format, &field_count, name, name_len, > + errcode, region) != 0) > + diag_raise(); > auto fields_guard = make_scoped_guard([=] { > space_def_destroy_fields(fields, field_count); > }); > diff --git a/src/box/index_def.c b/src/box/index_def.c > index 45c74d9..e0d54c0 100644 > --- a/src/box/index_def.c > +++ b/src/box/index_def.c > @@ -309,3 +309,63 @@ index_def_is_valid(struct index_def *index_def, const char *space_name) > } > return true; > } > + > +int > +index_opts_decode(struct index_opts *opts, const char *map, > + struct region *region) > +{ > + index_opts_create(opts); > + if (opts_decode(opts, index_opts_reg, &map, ER_WRONG_INDEX_OPTIONS, > + BOX_INDEX_FIELD_OPTS, region) != 0) { > + return -1; > + } > + if (opts->distance == rtree_index_distance_type_MAX) { > + diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, > + BOX_INDEX_FIELD_OPTS, "distance must be either "\ > + "'euclid' or 'manhattan'"); > + return -1; > + } > + if (opts->sql != NULL) { > + char *sql = strdup(opts->sql); > + if (sql == NULL) { > + opts->sql = NULL; > + diag_set(OutOfMemory, strlen(opts->sql) + 1, "strdup", > + "sql"); > + return -1; > + } > + opts->sql = sql; > + } > + if (opts->range_size <= 0) { > + diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, > + BOX_INDEX_FIELD_OPTS, > + "range_size must be greater than 0"); > + return -1; > + } > + if (opts->page_size <= 0 || opts->page_size > opts->range_size) { > + diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, > + BOX_INDEX_FIELD_OPTS, > + "page_size must be greater than 0 and " > + "less than or equal to range_size"); > + return -1; > + } > + if (opts->run_count_per_level <= 0) { > + diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, > + BOX_INDEX_FIELD_OPTS, > + "run_count_per_level must be greater than 0"); > + return -1; > + } > + if (opts->run_size_ratio <= 1) { > + diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, > + BOX_INDEX_FIELD_OPTS, > + "run_size_ratio must be greater than 1"); > + return -1; > + } > + if (opts->bloom_fpr <= 0 || opts->bloom_fpr > 1) { > + diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, > + BOX_INDEX_FIELD_OPTS, > + "bloom_fpr must be greater than 0 and " > + "less than or equal to 1"); > + return -1; > + } > + return 0; > +} > diff --git a/src/box/index_def.h b/src/box/index_def.h > index 48a7820..4c3625b 100644 > --- a/src/box/index_def.h > +++ b/src/box/index_def.h > @@ -358,6 +358,14 @@ index_def_cmp(const struct index_def *key1, const struct index_def *key2); > bool > index_def_is_valid(struct index_def *index_def, const char *space_name); > > +/** > + * Fill index_opts structure from opts field in tuple of space _index > + * Throw an error is unrecognized option. > + */ > +int > +index_opts_decode(struct index_opts *opts, const char *map, > + struct region *region); > + > #if defined(__cplusplus) > } /* extern "C" */ > > diff --git a/src/box/space_def.c b/src/box/space_def.c > index f5ca0b5..8780eec 100644 > --- a/src/box/space_def.c > +++ b/src/box/space_def.c > @@ -34,6 +34,10 @@ > #include "error.h" > #include "sql.h" > #include "msgpuck.h" > +#include "tuple_format.h" > +#include "schema_def.h" > +#include "identifier.h" > +#include "small/region.h" > > /** > * Make checks from msgpack. > @@ -351,3 +355,154 @@ error: > sql_expr_list_delete(db, checks); > return -1; > } > + > +/** > + * Decode field definition from MessagePack map. Format: > + * {name: , type: }. Type is optional. > + * @param[out] field Field to decode to. > + * @param data MessagePack map to decode. > + * @param space_name Name of a space, from which the field is got. > + * Used in error messages. > + * @param name_len Length of @a space_name. > + * @param errcode Error code to use for client errors. Either > + * create or modify space errors. > + * @param fieldno Field number to decode. Used in error messages. > + * @param region Region to allocate field name. > + * > + * @retval 0 Success. > + * @retval -1 Error. > + */ > +static inline int > +field_def_decode(struct field_def *field, const char **data, > + const char *space_name, uint32_t name_len, > + uint32_t errcode, uint32_t fieldno, struct region *region) > +{ > + if (mp_typeof(**data) != MP_MAP) { > + diag_set(ClientError, errcode, tt_cstr(space_name, name_len), > + tt_sprintf("field %d is not map", > + fieldno + TUPLE_INDEX_BASE)); > + return -1; > + } > + int count = mp_decode_map(data); > + *field = field_def_default; > + bool is_action_missing = true; > + uint32_t action_literal_len = strlen("nullable_action"); > + for (int i = 0; i < count; ++i) { > + if (mp_typeof(**data) != MP_STR) { > + diag_set(ClientError, errcode, > + tt_cstr(space_name, name_len), > + tt_sprintf("field %d format is not map"\ > + " with string keys", > + fieldno + TUPLE_INDEX_BASE)); > + return -1; > + } > + uint32_t key_len; > + const char *key = mp_decode_str(data, &key_len); > + if (opts_parse_key(field, field_def_reg, key, key_len, data, > + ER_WRONG_SPACE_FORMAT, > + fieldno + TUPLE_INDEX_BASE, region, > + true) != 0) > + return -1; > + if (is_action_missing && > + key_len == action_literal_len && > + memcmp(key, "nullable_action", action_literal_len) == 0) > + is_action_missing = false; > + } > + if (is_action_missing) { > + field->nullable_action = field->is_nullable ? > + ON_CONFLICT_ACTION_NONE > + : ON_CONFLICT_ACTION_DEFAULT; > + } > + if (field->name == NULL) { > + diag_set(ClientError, errcode, tt_cstr(space_name, name_len), > + tt_sprintf("field %d name is not specified", > + fieldno + TUPLE_INDEX_BASE)); > + return -1; > + } > + size_t field_name_len = strlen(field->name); > + if (field_name_len > BOX_NAME_MAX) { > + diag_set(ClientError, errcode, tt_cstr(space_name, name_len), > + tt_sprintf("field %d name is too long", > + fieldno + TUPLE_INDEX_BASE)); > + return -1; > + } > + if (identifier_check(field->name, field_name_len)) > + return -1; > + if (field->type == field_type_MAX) { > + diag_set(ClientError, errcode, tt_cstr(space_name, name_len), > + tt_sprintf("field %d has unknown field type", > + fieldno + TUPLE_INDEX_BASE)); > + return -1; > + } > + if (field->nullable_action == on_conflict_action_MAX) { > + diag_set(ClientError, errcode, tt_cstr(space_name, name_len), > + tt_sprintf("field %d has unknown field on conflict " > + "nullable action", > + fieldno + TUPLE_INDEX_BASE)); > + return -1; > + } > + if (!((field->is_nullable && field->nullable_action == > + ON_CONFLICT_ACTION_NONE) > + || (!field->is_nullable > + && field->nullable_action != ON_CONFLICT_ACTION_NONE))) { > + diag_set(ClientError, errcode, tt_cstr(space_name, name_len), > + tt_sprintf("field %d has conflicting nullability and " > + "nullable action properties", fieldno + > + TUPLE_INDEX_BASE)); > + return -1; > + } > + if (field->coll_id != COLL_NONE && > + field->type != FIELD_TYPE_STRING && > + field->type != FIELD_TYPE_SCALAR && > + field->type != FIELD_TYPE_ANY) { > + diag_set(ClientError, errcode, tt_cstr(space_name, name_len), > + tt_sprintf("collation is reasonable only for " > + "string, scalar and any fields")); > + return -1; > + } > + > + const char *dv = field->default_value; > + if (dv != NULL) { > + field->default_value_expr = sql_expr_compile(sql_get(), dv, > + strlen(dv)); > + if (field->default_value_expr == NULL) > + return -1; > + } > + return 0; > +} > + > +int > +space_format_decode(struct field_def **field, const char *data, > + uint32_t *out_count, const char *space_name, > + uint32_t name_len, uint32_t errcode, struct region *region) > +{ > + /* Type is checked by _space format. */ > + assert(mp_typeof(*data) == MP_ARRAY); > + *field = NULL; > + uint32_t count = mp_decode_array(&data); > + *out_count = count; > + if (count == 0) > + return 0; > + size_t size = count * sizeof(struct field_def); > + struct field_def *region_defs = > + (struct field_def *) region_alloc(region, size); > + if (region_defs == NULL) { > + diag_set(OutOfMemory, size, "region_alloc", "region_defs"); > + return -1; > + } > + /* > + * Nullify to prevent a case when decoding will fail in > + * the middle and space_def_destroy_fields() below will > + * work with garbage pointers. > + */ > + memset(region_defs, 0, size); > + for (uint32_t i = 0; i < count; ++i) { > + if (field_def_decode(®ion_defs[i], &data, space_name, > + name_len, errcode, i, region) != 0) { > + space_def_destroy_fields(region_defs, count); > + return -1; > + } > + } > + *field = region_defs; > + return 0; > +} > diff --git a/src/box/space_def.h b/src/box/space_def.h > index 0d1e902..7a01b92 100644 > --- a/src/box/space_def.h > +++ b/src/box/space_def.h > @@ -182,6 +182,24 @@ space_def_sizeof(uint32_t name_len, const struct field_def *fields, > uint32_t field_count, uint32_t *names_offset, > uint32_t *fields_offset, uint32_t *def_expr_offset); > > +/** > + * Decode MessagePack array of fields. > + * @param[out] field Field to decode to. > + * @param data MessagePack array of fields. > + * @param[out] out_count Length of a result array. > + * @param space_name Space name to use in error messages. > + * @param name_len Length of @a space_name. > + * @param errcode Errcode for client errors. > + * @param region Region to allocate result array. > + * > + * @retval 0 Success. > + * @retval -1 Error. > + */ > +int > +space_format_decode(struct field_def **field, const char *data, > + uint32_t *out_count, const char *space_name, > + uint32_t name_len, uint32_t errcode, struct region *region); > + > #if defined(__cplusplus) > } /* extern "C" */ > >