[tarantool-patches] Re: [PATCH v2 4/7] box: bove some decode functions from alter.cc
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Jul 18 12:24:15 MSK 2018
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 at 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 <new> /* for placement new */
> -#include <stdio.h> /* snprintf() */
> -#include <ctype.h>
> #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: <string>, type: <string>}. 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: <string>, type: <string>}. 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" */
>
>
More information about the Tarantool-patches
mailing list