* [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format @ 2020-04-02 10:41 imeevma 2020-04-03 13:00 ` Nikita Pettik 2020-04-04 18:34 ` Vladislav Shpilevoy 0 siblings, 2 replies; 7+ messages in thread From: imeevma @ 2020-04-02 10:41 UTC (permalink / raw) To: v.shpilevoy, tsafin, tarantool-patches This patch allows to specify field types in ephemeral space format. Prior to this patch, all fields had a SCALAR field type. Closes #3841 --- https://github.com/tarantool/tarantool/issues/3841 https://github.com/tarantool/tarantool/tree/imeevma/gh-3841-format-for-ephemeral-space @ChangeLog - Format of ephemeral spaces can have field with types other that SCALAR now (gh-3841). src/box/space_def.c | 5 ++- src/box/space_def.h | 2 +- src/box/sql.c | 83 +++++++++++++++++++++++++++++++------------------- src/box/tuple_format.c | 22 ++++++------- 4 files changed, 64 insertions(+), 48 deletions(-) diff --git a/src/box/space_def.c b/src/box/space_def.c index 6611312..5af0f38 100644 --- a/src/box/space_def.c +++ b/src/box/space_def.c @@ -233,7 +233,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, } struct space_def* -space_def_new_ephemeral(uint32_t field_count) +space_def_new_ephemeral(uint32_t field_count, struct field_def *fields) { struct space_opts opts = space_opts_default; opts.is_temporary = true; @@ -242,8 +242,7 @@ space_def_new_ephemeral(uint32_t field_count) "ephemeral", strlen("ephemeral"), "memtx", strlen("memtx"), - &opts, &field_def_default, - 0); + &opts, fields, field_count); return space_def; } diff --git a/src/box/space_def.h b/src/box/space_def.h index ac6d226..e7b0c28 100644 --- a/src/box/space_def.h +++ b/src/box/space_def.h @@ -177,7 +177,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, * @retval Space definition. */ struct space_def * -space_def_new_ephemeral(uint32_t field_count); +space_def_new_ephemeral(uint32_t field_count, struct field_def *fields); /** * Size of the space_def. diff --git a/src/box/sql.c b/src/box/sql.c index 1256df8..27089bd 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -330,58 +330,77 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) return NULL; } - struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc, - sizeof(*ephemer_key_parts) * field_count); - if (ephemer_key_parts == NULL) { - diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count, - "region", "key parts"); - return NULL; + struct region *region = &fiber()->gc; + /* + * Name of the fields will be "_COLUMN_1", "_COLUMN_2" + * and so on. Since number of columns no more than 2000, + * length of each name is no more than strlen("_COLUMN_") + * + 5. + */ + assert(SQL_MAX_COLUMN < 9999); + uint32_t name_len = sizeof("_COLUMN_") + 5; + uint32_t size = field_count * (sizeof(struct field_def) + name_len + + sizeof(struct key_part_def)); + struct field_def *fields = region_alloc(region, size); + struct key_part_def *parts = (void *)fields + + field_count * sizeof(struct field_def); + char *names = (char *)parts + field_count * sizeof(struct key_part_def); + for (uint32_t i = 0; i < field_count; ++i) { + struct field_def *field = &fields[i]; + field->name = names; + names += name_len; + sprintf(field->name, "_COLUMN_%d", i); + field->is_nullable = true; + field->nullable_action = ON_CONFLICT_ACTION_NONE; + field->default_value = NULL; + field->default_value_expr = NULL; + if (def != NULL && i < def->part_count) { + assert(def->parts[i].type < field_type_MAX); + field->type = def->parts[i].type; + field->coll_id = def->parts[i].coll_id; + } else { + field->coll_id = COLL_NONE; + field->type = FIELD_TYPE_SCALAR; + } } + for (uint32_t i = 0; i < field_count; ++i) { - struct key_part_def *part = &ephemer_key_parts[i]; + struct key_part_def *part = &parts[i]; part->fieldno = i; part->nullable_action = ON_CONFLICT_ACTION_NONE; part->is_nullable = true; part->sort_order = SORT_ORDER_ASC; part->path = NULL; - if (def != NULL && i < def->part_count) { - assert(def->parts[i].type < field_type_MAX); - part->type = def->parts[i].type; - part->coll_id = def->parts[i].coll_id; - } else { - part->coll_id = COLL_NONE; - part->type = FIELD_TYPE_SCALAR; - } + part->type = fields[i].type; + part->coll_id = fields[i].coll_id; } - struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts, - field_count, false); - if (ephemer_key_def == NULL) + struct key_def *key_def = key_def_new(parts, field_count, false); + if (key_def == NULL) return NULL; - struct index_def *ephemer_index_def = + struct index_def *index_def = index_def_new(0, 0, "ephemer_idx", strlen("ephemer_idx"), TREE, - &index_opts_default, ephemer_key_def, NULL); - key_def_delete(ephemer_key_def); - if (ephemer_index_def == NULL) + &index_opts_default, key_def, NULL); + key_def_delete(key_def); + if (index_def == NULL) return NULL; struct rlist key_list; rlist_create(&key_list); - rlist_add_entry(&key_list, ephemer_index_def, link); + rlist_add_entry(&key_list, index_def, link); - struct space_def *ephemer_space_def = - space_def_new_ephemeral(field_count); - if (ephemer_space_def == NULL) { - index_def_delete(ephemer_index_def); + struct space_def *space_def = space_def_new_ephemeral(field_count, + fields); + if (space_def == NULL) { + index_def_delete(index_def); return NULL; } - struct space *ephemer_new_space = space_new_ephemeral(ephemer_space_def, - &key_list); - index_def_delete(ephemer_index_def); - space_def_delete(ephemer_space_def); + struct space *space = space_new_ephemeral(space_def, &key_list); + index_def_delete(index_def); + space_def_delete(space_def); - return ephemer_new_space; + return space; } int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple, diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index 312c966..beaeb0f 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format) * dictionary will never be altered. If it can, then alter can * affect another space, which shares a format with one which is * altered. + * + * The only way to change the format of the space is to recreate + * space with the new format inside of BOX. Since there is no + * mechanism for recreating the ephemeral space, we need not worry + * about changing the format of the ephemeral space. + * * @param p_format Double pointer to format. It is updated with * hashed value, if corresponding format was found * in hash table @@ -709,13 +715,7 @@ static bool tuple_format_reuse(struct tuple_format **p_format) { struct tuple_format *format = *p_format; - if (!format->is_ephemeral) - return false; - /* - * These fields do not participate in hashing. - * Make sure they're unset. - */ - assert(format->dict->name_count == 0); + assert(format->is_ephemeral); assert(format->is_temporary); mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, NULL); @@ -739,9 +739,7 @@ tuple_format_reuse(struct tuple_format **p_format) static int tuple_format_add_to_hash(struct tuple_format *format) { - if(!format->is_ephemeral) - return 0; - assert(format->dict->name_count == 0); + assert(format->is_ephemeral); assert(format->is_temporary); mh_int_t key = mh_tuple_format_put(tuple_formats_hash, (const struct tuple_format **)&format, @@ -795,11 +793,11 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, if (tuple_format_create(format, keys, key_count, space_fields, space_field_count) < 0) goto err; - if (tuple_format_reuse(&format)) + if (is_ephemeral && tuple_format_reuse(&format)) return format; if (tuple_format_register(format) < 0) goto err; - if (tuple_format_add_to_hash(format) < 0) { + if (is_ephemeral && tuple_format_add_to_hash(format) < 0) { tuple_format_deregister(format); goto err; } -- 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format 2020-04-02 10:41 [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format imeevma @ 2020-04-03 13:00 ` Nikita Pettik 2020-04-04 18:34 ` Vladislav Shpilevoy 1 sibling, 0 replies; 7+ messages in thread From: Nikita Pettik @ 2020-04-03 13:00 UTC (permalink / raw) To: imeevma; +Cc: tarantool-patches, v.shpilevoy On 02 Apr 13:41, imeevma@tarantool.org wrote: > This patch allows to specify field types in ephemeral space format. > Prior to this patch, all fields had a SCALAR field type. > Closes #3841 > --- > https://github.com/tarantool/tarantool/issues/3841 > https://github.com/tarantool/tarantool/tree/imeevma/gh-3841-format-for-ephemeral-space > > @ChangeLog > - Format of ephemeral spaces can have field with types other that > SCALAR now (gh-3841). > This change is not visible to users. Should we extend changelog with corresponding entry? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format 2020-04-02 10:41 [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format imeevma 2020-04-03 13:00 ` Nikita Pettik @ 2020-04-04 18:34 ` Vladislav Shpilevoy 2020-04-10 0:04 ` Mergen Imeev 1 sibling, 1 reply; 7+ messages in thread From: Vladislav Shpilevoy @ 2020-04-04 18:34 UTC (permalink / raw) To: imeevma, tsafin, tarantool-patches Hi! Thanks for the patch! On 02/04/2020 12:41, imeevma@tarantool.org wrote: > This patch allows to specify field types in ephemeral space format. > Prior to this patch, all fields had a SCALAR field type. 1. Why is it needed? (I see description in the ticket, but it should be in the commit message too). I copy paste it here: > During ephemeral table creation via SQL all parts are declared as SCALAR: see sql_ephemeral_space_create. However, in most cases it is possible to fill it with specific type - it may speed up internal routine connected with tuples comparison. And the real question is how is it going to speedup and what exactly? If the subject are internal comparators, they don't depend on format. They depend only on key definitions, and the latter are ok so far, they have types. > Closes #3841 > --- > https://github.com/tarantool/tarantool/issues/3841 > https://github.com/tarantool/tarantool/tree/imeevma/gh-3841-format-for-ephemeral-space > > diff --git a/src/box/space_def.h b/src/box/space_def.h > index ac6d226..e7b0c28 100644 > --- a/src/box/space_def.h > +++ b/src/box/space_def.h > @@ -177,7 +177,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, > * @retval Space definition. > */ > struct space_def * > -space_def_new_ephemeral(uint32_t field_count); > +space_def_new_ephemeral(uint32_t field_count, struct field_def *fields); 2. The comment is not updated. > /** > * Size of the space_def. > diff --git a/src/box/sql.c b/src/box/sql.c > index 1256df8..27089bd 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -330,58 +330,77 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > return NULL; > } > > - struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc, > - sizeof(*ephemer_key_parts) * field_count); > - if (ephemer_key_parts == NULL) { > - diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count, > - "region", "key parts"); > - return NULL; > + struct region *region = &fiber()->gc; > + /* > + * Name of the fields will be "_COLUMN_1", "_COLUMN_2" > + * and so on. Since number of columns no more than 2000, 3. You said no more than 2000, but below check no more than 9999. Why? > + * length of each name is no more than strlen("_COLUMN_") > + * + 5. > + */ > + assert(SQL_MAX_COLUMN < 9999); > + uint32_t name_len = sizeof("_COLUMN_") + 5; > + uint32_t size = field_count * (sizeof(struct field_def) + name_len + > + sizeof(struct key_part_def)); > + struct field_def *fields = region_alloc(region, size); > + struct key_part_def *parts = (void *)fields + > + field_count * sizeof(struct field_def); > + char *names = (char *)parts + field_count * sizeof(struct key_part_def); > + for (uint32_t i = 0; i < field_count; ++i) { 4. Why do you need a second cycle for this? Can the fields be initialized in the same cycle as key parts? > + struct field_def *field = &fields[i]; > + field->name = names; > + names += name_len; > + sprintf(field->name, "_COLUMN_%d", i); > + field->is_nullable = true; > + field->nullable_action = ON_CONFLICT_ACTION_NONE; > + field->default_value = NULL; > + field->default_value_expr = NULL; > + if (def != NULL && i < def->part_count) { > + assert(def->parts[i].type < field_type_MAX); > + field->type = def->parts[i].type; > + field->coll_id = def->parts[i].coll_id; > + } else { > + field->coll_id = COLL_NONE; > + field->type = FIELD_TYPE_SCALAR; > + } > } > + > for (uint32_t i = 0; i < field_count; ++i) { > - struct key_part_def *part = &ephemer_key_parts[i]; > + struct key_part_def *part = &parts[i]; > part->fieldno = i; > part->nullable_action = ON_CONFLICT_ACTION_NONE; > part->is_nullable = true; > part->sort_order = SORT_ORDER_ASC; > part->path = NULL; > - if (def != NULL && i < def->part_count) { > - assert(def->parts[i].type < field_type_MAX); > - part->type = def->parts[i].type; > - part->coll_id = def->parts[i].coll_id; > - } else { > - part->coll_id = COLL_NONE; > - part->type = FIELD_TYPE_SCALAR; > - } > + part->type = fields[i].type; > + part->coll_id = fields[i].coll_id; > } > - struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts, > - field_count, false); > - if (ephemer_key_def == NULL) > + struct key_def *key_def = key_def_new(parts, field_count, false); > + if (key_def == NULL) > return NULL; > > - struct index_def *ephemer_index_def = > + struct index_def *index_def = > index_def_new(0, 0, "ephemer_idx", strlen("ephemer_idx"), TREE, > - &index_opts_default, ephemer_key_def, NULL); > - key_def_delete(ephemer_key_def); > - if (ephemer_index_def == NULL) > + &index_opts_default, key_def, NULL); > + key_def_delete(key_def); > + if (index_def == NULL) > return NULL; > > struct rlist key_list; > rlist_create(&key_list); > - rlist_add_entry(&key_list, ephemer_index_def, link); > + rlist_add_entry(&key_list, index_def, link); > > - struct space_def *ephemer_space_def = > - space_def_new_ephemeral(field_count); > - if (ephemer_space_def == NULL) { > - index_def_delete(ephemer_index_def); > + struct space_def *space_def = space_def_new_ephemeral(field_count, > + fields); > + if (space_def == NULL) { > + index_def_delete(index_def); > return NULL; > } > > - struct space *ephemer_new_space = space_new_ephemeral(ephemer_space_def, > - &key_list); > - index_def_delete(ephemer_index_def); > - space_def_delete(ephemer_space_def); > + struct space *space = space_new_ephemeral(space_def, &key_list); > + index_def_delete(index_def); > + space_def_delete(space_def); > > - return ephemer_new_space; > + return space; 5. Seems like you could keep the old names, and avoid half of these changes. > } > > int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple, > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > index 312c966..beaeb0f 100644 > --- a/src/box/tuple_format.c > +++ b/src/box/tuple_format.c 6. Looks like all the diff in this file can be dropped. Because it does not contain any functional changes. Right? You just turned 'if' into assert + the same 'if' a bit earlier. > @@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format) > * dictionary will never be altered. If it can, then alter can > * affect another space, which shares a format with one which is > * altered. > + * > + * The only way to change the format of the space is to recreate > + * space with the new format inside of BOX. Since there is no > + * mechanism for recreating the ephemeral space, we need not worry > + * about changing the format of the ephemeral space. > + * > * @param p_format Double pointer to format. It is updated with > * hashed value, if corresponding format was found > * in hash table > @@ -709,13 +715,7 @@ static bool > tuple_format_reuse(struct tuple_format **p_format) > { > struct tuple_format *format = *p_format; > - if (!format->is_ephemeral) > - return false; > - /* > - * These fields do not participate in hashing. > - * Make sure they're unset. > - */ > - assert(format->dict->name_count == 0); > + assert(format->is_ephemeral); > assert(format->is_temporary); > mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, > NULL); > @@ -739,9 +739,7 @@ tuple_format_reuse(struct tuple_format **p_format) > static int > tuple_format_add_to_hash(struct tuple_format *format) > { > - if(!format->is_ephemeral) > - return 0; > - assert(format->dict->name_count == 0); > + assert(format->is_ephemeral); > assert(format->is_temporary); > mh_int_t key = mh_tuple_format_put(tuple_formats_hash, > (const struct tuple_format **)&format, > @@ -795,11 +793,11 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, > if (tuple_format_create(format, keys, key_count, space_fields, > space_field_count) < 0) > goto err; > - if (tuple_format_reuse(&format)) > + if (is_ephemeral && tuple_format_reuse(&format)) > return format; > if (tuple_format_register(format) < 0) > goto err; > - if (tuple_format_add_to_hash(format) < 0) { > + if (is_ephemeral && tuple_format_add_to_hash(format) < 0) { > tuple_format_deregister(format); > goto err; > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format 2020-04-04 18:34 ` Vladislav Shpilevoy @ 2020-04-10 0:04 ` Mergen Imeev 2020-04-10 19:16 ` Vladislav Shpilevoy 0 siblings, 1 reply; 7+ messages in thread From: Mergen Imeev @ 2020-04-10 0:04 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hi! Thank you for review! My answers, diff and new patch below. On Sat, Apr 04, 2020 at 08:34:32PM +0200, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > On 02/04/2020 12:41, imeevma@tarantool.org wrote: > > This patch allows to specify field types in ephemeral space format. > > Prior to this patch, all fields had a SCALAR field type. > > 1. Why is it needed? (I see description in the ticket, but it should > be in the commit message too). > > I copy paste it here: > > > During ephemeral table creation via SQL all parts are declared as SCALAR: see sql_ephemeral_space_create. However, in most cases it is possible to fill it with specific type - it may speed up internal routine connected with tuples comparison. > I am not sure that it will speed up something, at least at this patch. But this patch is needed for #4692 and #4256. > And the real question is how is it going to speedup and what exactly? > If the subject are internal comparators, they don't depend on format. > They depend only on key definitions, and the latter are ok so far, they > have types. > > > Closes #3841 > > --- > > https://github.com/tarantool/tarantool/issues/3841 > > https://github.com/tarantool/tarantool/tree/imeevma/gh-3841-format-for-ephemeral-space > > > > diff --git a/src/box/space_def.h b/src/box/space_def.h > > index ac6d226..e7b0c28 100644 > > --- a/src/box/space_def.h > > +++ b/src/box/space_def.h > > @@ -177,7 +177,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, > > * @retval Space definition. > > */ > > struct space_def * > > -space_def_new_ephemeral(uint32_t field_count); > > +space_def_new_ephemeral(uint32_t field_count, struct field_def *fields); > > 2. The comment is not updated. > Fixed. > > /** > > * Size of the space_def. > > diff --git a/src/box/sql.c b/src/box/sql.c > > index 1256df8..27089bd 100644 > > --- a/src/box/sql.c > > +++ b/src/box/sql.c > > @@ -330,58 +330,77 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > > return NULL; > > } > > > > - struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc, > > - sizeof(*ephemer_key_parts) * field_count); > > - if (ephemer_key_parts == NULL) { > > - diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count, > > - "region", "key parts"); > > - return NULL; > > + struct region *region = &fiber()->gc; > > + /* > > + * Name of the fields will be "_COLUMN_1", "_COLUMN_2" > > + * and so on. Since number of columns no more than 2000, > > 3. You said no more than 2000, but below check no more than 9999. > Why? > Important here is that number of figures is no more than 4. Still, I fixed that. > > + * length of each name is no more than strlen("_COLUMN_") > > + * + 5. > > + */ > > + assert(SQL_MAX_COLUMN < 9999); > > + uint32_t name_len = sizeof("_COLUMN_") + 5; > > + uint32_t size = field_count * (sizeof(struct field_def) + name_len + > > + sizeof(struct key_part_def)); > > + struct field_def *fields = region_alloc(region, size); > > + struct key_part_def *parts = (void *)fields + > > + field_count * sizeof(struct field_def); > > + char *names = (char *)parts + field_count * sizeof(struct key_part_def); > > + for (uint32_t i = 0; i < field_count; ++i) { > > 4. Why do you need a second cycle for this? Can the fields > be initialized in the same cycle as key parts? > They can, but I plan to change the definition of the index in some cases, which is easier to do when they are in two different cycles. The only problem was that index had to have all fields since it worked as format in some cases. This is not so now since format is fixed. Still, I changed assert. > > + struct field_def *field = &fields[i]; > > + field->name = names; > > + names += name_len; > > + sprintf(field->name, "_COLUMN_%d", i); > > + field->is_nullable = true; > > + field->nullable_action = ON_CONFLICT_ACTION_NONE; > > + field->default_value = NULL; > > + field->default_value_expr = NULL; > > + if (def != NULL && i < def->part_count) { > > + assert(def->parts[i].type < field_type_MAX); > > + field->type = def->parts[i].type; > > + field->coll_id = def->parts[i].coll_id; > > + } else { > > + field->coll_id = COLL_NONE; > > + field->type = FIELD_TYPE_SCALAR; > > + } > > } > > + > > for (uint32_t i = 0; i < field_count; ++i) { > > - struct key_part_def *part = &ephemer_key_parts[i]; > > + struct key_part_def *part = &parts[i]; > > part->fieldno = i; > > part->nullable_action = ON_CONFLICT_ACTION_NONE; > > part->is_nullable = true; > > part->sort_order = SORT_ORDER_ASC; > > part->path = NULL; > > - if (def != NULL && i < def->part_count) { > > - assert(def->parts[i].type < field_type_MAX); > > - part->type = def->parts[i].type; > > - part->coll_id = def->parts[i].coll_id; > > - } else { > > - part->coll_id = COLL_NONE; > > - part->type = FIELD_TYPE_SCALAR; > > - } > > + part->type = fields[i].type; > > + part->coll_id = fields[i].coll_id; > > } > > - struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts, > > - field_count, false); > > - if (ephemer_key_def == NULL) > > + struct key_def *key_def = key_def_new(parts, field_count, false); > > + if (key_def == NULL) > > return NULL; > > > > - struct index_def *ephemer_index_def = > > + struct index_def *index_def = > > index_def_new(0, 0, "ephemer_idx", strlen("ephemer_idx"), TREE, > > - &index_opts_default, ephemer_key_def, NULL); > > - key_def_delete(ephemer_key_def); > > - if (ephemer_index_def == NULL) > > + &index_opts_default, key_def, NULL); > > + key_def_delete(key_def); > > + if (index_def == NULL) > > return NULL; > > > > struct rlist key_list; > > rlist_create(&key_list); > > - rlist_add_entry(&key_list, ephemer_index_def, link); > > + rlist_add_entry(&key_list, index_def, link); > > > > - struct space_def *ephemer_space_def = > > - space_def_new_ephemeral(field_count); > > - if (ephemer_space_def == NULL) { > > - index_def_delete(ephemer_index_def); > > + struct space_def *space_def = space_def_new_ephemeral(field_count, > > + fields); > > + if (space_def == NULL) { > > + index_def_delete(index_def); > > return NULL; > > } > > > > - struct space *ephemer_new_space = space_new_ephemeral(ephemer_space_def, > > - &key_list); > > - index_def_delete(ephemer_index_def); > > - space_def_delete(ephemer_space_def); > > + struct space *space = space_new_ephemeral(space_def, &key_list); > > + index_def_delete(index_def); > > + space_def_delete(space_def); > > > > - return ephemer_new_space; > > + return space; > > 5. Seems like you could keep the old names, and avoid half of these > changes. > Fixed, returned old names. > > } > > > > int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple, > > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > > index 312c966..beaeb0f 100644 > > --- a/src/box/tuple_format.c > > +++ b/src/box/tuple_format.c > > 6. Looks like all the diff in this file can be dropped. Because it > does not contain any functional changes. Right? You just turned > 'if' into assert + the same 'if' a bit earlier. > Not exactly so. Since now we have dict in format of ephemeral space old assert throws an error each time ephemeral space is used. New 'if' allows to remove this assert without changing general logic. > > @@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format) > > * dictionary will never be altered. If it can, then alter can > > * affect another space, which shares a format with one which is > > * altered. > > + * > > + * The only way to change the format of the space is to recreate > > + * space with the new format inside of BOX. Since there is no > > + * mechanism for recreating the ephemeral space, we need not worry > > + * about changing the format of the ephemeral space. > > + * > > * @param p_format Double pointer to format. It is updated with > > * hashed value, if corresponding format was found > > * in hash table > > @@ -709,13 +715,7 @@ static bool > > tuple_format_reuse(struct tuple_format **p_format) > > { > > struct tuple_format *format = *p_format; > > - if (!format->is_ephemeral) > > - return false; > > - /* > > - * These fields do not participate in hashing. > > - * Make sure they're unset. > > - */ > > - assert(format->dict->name_count == 0); > > + assert(format->is_ephemeral); > > assert(format->is_temporary); > > mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, > > NULL); > > @@ -739,9 +739,7 @@ tuple_format_reuse(struct tuple_format **p_format) > > static int > > tuple_format_add_to_hash(struct tuple_format *format) > > { > > - if(!format->is_ephemeral) > > - return 0; > > - assert(format->dict->name_count == 0); > > + assert(format->is_ephemeral); > > assert(format->is_temporary); > > mh_int_t key = mh_tuple_format_put(tuple_formats_hash, > > (const struct tuple_format **)&format, > > @@ -795,11 +793,11 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, > > if (tuple_format_create(format, keys, key_count, space_fields, > > space_field_count) < 0) > > goto err; > > - if (tuple_format_reuse(&format)) > > + if (is_ephemeral && tuple_format_reuse(&format)) > > return format; > > if (tuple_format_register(format) < 0) > > goto err; > > - if (tuple_format_add_to_hash(format) < 0) { > > + if (is_ephemeral && tuple_format_add_to_hash(format) < 0) { > > tuple_format_deregister(format); > > goto err; > > } > > Diff: From f5e07106409a3183d81b57f4dd44bb868848021d Mon Sep 17 00:00:00 2001 From: Mergen Imeev <imeevma@gmail.com> Date: Fri, 10 Apr 2020 02:42:54 +0300 Subject: [PATCH] Review fix diff --git a/src/box/space_def.h b/src/box/space_def.h index e7b0c28..61bbdb4 100644 --- a/src/box/space_def.h +++ b/src/box/space_def.h @@ -173,6 +173,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, /** * Create a new ephemeral space definition. * @param field_count Number of fields in the space. + * @param fields Field definitions. * * @retval Space definition. */ diff --git a/src/box/sql.c b/src/box/sql.c index 27089bd..288c4fa 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -337,14 +337,15 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) * length of each name is no more than strlen("_COLUMN_") * + 5. */ - assert(SQL_MAX_COLUMN < 9999); + assert(SQL_MAX_COLUMN <= 2000); uint32_t name_len = sizeof("_COLUMN_") + 5; uint32_t size = field_count * (sizeof(struct field_def) + name_len + sizeof(struct key_part_def)); struct field_def *fields = region_alloc(region, size); - struct key_part_def *parts = (void *)fields + + struct key_part_def *ephemer_key_parts = (void *)fields + field_count * sizeof(struct field_def); - char *names = (char *)parts + field_count * sizeof(struct key_part_def); + char *names = (char *)ephemer_key_parts + + field_count * sizeof(struct key_part_def); for (uint32_t i = 0; i < field_count; ++i) { struct field_def *field = &fields[i]; field->name = names; @@ -365,7 +366,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) } for (uint32_t i = 0; i < field_count; ++i) { - struct key_part_def *part = &parts[i]; + struct key_part_def *part = &ephemer_key_parts[i]; part->fieldno = i; part->nullable_action = ON_CONFLICT_ACTION_NONE; part->is_nullable = true; @@ -374,33 +375,35 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) part->type = fields[i].type; part->coll_id = fields[i].coll_id; } - struct key_def *key_def = key_def_new(parts, field_count, false); - if (key_def == NULL) + struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts, + field_count, false); + if (ephemer_key_def == NULL) return NULL; - struct index_def *index_def = + struct index_def *ephemer_index_def = index_def_new(0, 0, "ephemer_idx", strlen("ephemer_idx"), TREE, - &index_opts_default, key_def, NULL); - key_def_delete(key_def); - if (index_def == NULL) + &index_opts_default, ephemer_key_def, NULL); + key_def_delete(ephemer_key_def); + if (ephemer_index_def == NULL) return NULL; struct rlist key_list; rlist_create(&key_list); - rlist_add_entry(&key_list, index_def, link); + rlist_add_entry(&key_list, ephemer_index_def, link); - struct space_def *space_def = space_def_new_ephemeral(field_count, - fields); - if (space_def == NULL) { - index_def_delete(index_def); + struct space_def *ephemer_space_def = + space_def_new_ephemeral(field_count, fields); + if (ephemer_space_def == NULL) { + index_def_delete(ephemer_index_def); return NULL; } - struct space *space = space_new_ephemeral(space_def, &key_list); - index_def_delete(index_def); - space_def_delete(space_def); + struct space *ephemer_new_space = space_new_ephemeral(ephemer_space_def, + &key_list); + index_def_delete(ephemer_index_def); + space_def_delete(ephemer_space_def); - return space; + return ephemer_new_space; } int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple, New patch: From 2cf0d4749e7858b533225374f62da8556a51bcd9 Mon Sep 17 00:00:00 2001 From: Mergen Imeev <imeevma@gmail.com> Date: Thu, 23 Jan 2020 18:34:00 +0300 Subject: [PATCH] sql: specify field types in ephemeral space format This patch allows to specify field types in ephemeral space format. Prior to this patch, all fields had a SCALAR field type. This patch simplifies the structure of the ephemeral space a little. Needed for #4256 Needed for #4692 Closes #3841 diff --git a/src/box/space_def.c b/src/box/space_def.c index 6611312..5af0f38 100644 --- a/src/box/space_def.c +++ b/src/box/space_def.c @@ -233,7 +233,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, } struct space_def* -space_def_new_ephemeral(uint32_t field_count) +space_def_new_ephemeral(uint32_t field_count, struct field_def *fields) { struct space_opts opts = space_opts_default; opts.is_temporary = true; @@ -242,8 +242,7 @@ space_def_new_ephemeral(uint32_t field_count) "ephemeral", strlen("ephemeral"), "memtx", strlen("memtx"), - &opts, &field_def_default, - 0); + &opts, fields, field_count); return space_def; } diff --git a/src/box/space_def.h b/src/box/space_def.h index ac6d226..61bbdb4 100644 --- a/src/box/space_def.h +++ b/src/box/space_def.h @@ -173,11 +173,12 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, /** * Create a new ephemeral space definition. * @param field_count Number of fields in the space. + * @param fields Field definitions. * * @retval Space definition. */ struct space_def * -space_def_new_ephemeral(uint32_t field_count); +space_def_new_ephemeral(uint32_t field_count, struct field_def *fields); /** * Size of the space_def. diff --git a/src/box/sql.c b/src/box/sql.c index 1256df8..288c4fa 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -330,13 +330,41 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) return NULL; } - struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc, - sizeof(*ephemer_key_parts) * field_count); - if (ephemer_key_parts == NULL) { - diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count, - "region", "key parts"); - return NULL; + struct region *region = &fiber()->gc; + /* + * Name of the fields will be "_COLUMN_1", "_COLUMN_2" + * and so on. Since number of columns no more than 2000, + * length of each name is no more than strlen("_COLUMN_") + * + 5. + */ + assert(SQL_MAX_COLUMN <= 2000); + uint32_t name_len = sizeof("_COLUMN_") + 5; + uint32_t size = field_count * (sizeof(struct field_def) + name_len + + sizeof(struct key_part_def)); + struct field_def *fields = region_alloc(region, size); + struct key_part_def *ephemer_key_parts = (void *)fields + + field_count * sizeof(struct field_def); + char *names = (char *)ephemer_key_parts + + field_count * sizeof(struct key_part_def); + for (uint32_t i = 0; i < field_count; ++i) { + struct field_def *field = &fields[i]; + field->name = names; + names += name_len; + sprintf(field->name, "_COLUMN_%d", i); + field->is_nullable = true; + field->nullable_action = ON_CONFLICT_ACTION_NONE; + field->default_value = NULL; + field->default_value_expr = NULL; + if (def != NULL && i < def->part_count) { + assert(def->parts[i].type < field_type_MAX); + field->type = def->parts[i].type; + field->coll_id = def->parts[i].coll_id; + } else { + field->coll_id = COLL_NONE; + field->type = FIELD_TYPE_SCALAR; + } } + for (uint32_t i = 0; i < field_count; ++i) { struct key_part_def *part = &ephemer_key_parts[i]; part->fieldno = i; @@ -344,14 +372,8 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) part->is_nullable = true; part->sort_order = SORT_ORDER_ASC; part->path = NULL; - if (def != NULL && i < def->part_count) { - assert(def->parts[i].type < field_type_MAX); - part->type = def->parts[i].type; - part->coll_id = def->parts[i].coll_id; - } else { - part->coll_id = COLL_NONE; - part->type = FIELD_TYPE_SCALAR; - } + part->type = fields[i].type; + part->coll_id = fields[i].coll_id; } struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts, field_count, false); @@ -370,7 +392,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) rlist_add_entry(&key_list, ephemer_index_def, link); struct space_def *ephemer_space_def = - space_def_new_ephemeral(field_count); + space_def_new_ephemeral(field_count, fields); if (ephemer_space_def == NULL) { index_def_delete(ephemer_index_def); return NULL; diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index 312c966..beaeb0f 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format) * dictionary will never be altered. If it can, then alter can * affect another space, which shares a format with one which is * altered. + * + * The only way to change the format of the space is to recreate + * space with the new format inside of BOX. Since there is no + * mechanism for recreating the ephemeral space, we need not worry + * about changing the format of the ephemeral space. + * * @param p_format Double pointer to format. It is updated with * hashed value, if corresponding format was found * in hash table @@ -709,13 +715,7 @@ static bool tuple_format_reuse(struct tuple_format **p_format) { struct tuple_format *format = *p_format; - if (!format->is_ephemeral) - return false; - /* - * These fields do not participate in hashing. - * Make sure they're unset. - */ - assert(format->dict->name_count == 0); + assert(format->is_ephemeral); assert(format->is_temporary); mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, NULL); @@ -739,9 +739,7 @@ tuple_format_reuse(struct tuple_format **p_format) static int tuple_format_add_to_hash(struct tuple_format *format) { - if(!format->is_ephemeral) - return 0; - assert(format->dict->name_count == 0); + assert(format->is_ephemeral); assert(format->is_temporary); mh_int_t key = mh_tuple_format_put(tuple_formats_hash, (const struct tuple_format **)&format, @@ -795,11 +793,11 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, if (tuple_format_create(format, keys, key_count, space_fields, space_field_count) < 0) goto err; - if (tuple_format_reuse(&format)) + if (is_ephemeral && tuple_format_reuse(&format)) return format; if (tuple_format_register(format) < 0) goto err; - if (tuple_format_add_to_hash(format) < 0) { + if (is_ephemeral && tuple_format_add_to_hash(format) < 0) { tuple_format_deregister(format); goto err; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format 2020-04-10 0:04 ` Mergen Imeev @ 2020-04-10 19:16 ` Vladislav Shpilevoy 2020-04-10 21:52 ` Mergen Imeev 0 siblings, 1 reply; 7+ messages in thread From: Vladislav Shpilevoy @ 2020-04-10 19:16 UTC (permalink / raw) To: Mergen Imeev; +Cc: tarantool-patches Hi! Thanks for the fixes! See 2 comments below. > diff --git a/src/box/sql.c b/src/box/sql.c > index 27089bd..288c4fa 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -337,14 +337,15 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > * length of each name is no more than strlen("_COLUMN_") > * + 5. > */ > - assert(SQL_MAX_COLUMN < 9999); > + assert(SQL_MAX_COLUMN <= 2000); > uint32_t name_len = sizeof("_COLUMN_") + 5; 1. sizeof("_COLUMN_") != strlen("_COLUMN_"). The latter is 8, the former is 9. Better use strlen(). > uint32_t size = field_count * (sizeof(struct field_def) + name_len + > sizeof(struct key_part_def)); > struct field_def *fields = region_alloc(region, size); > - struct key_part_def *parts = (void *)fields + > + struct key_part_def *ephemer_key_parts = (void *)fields + > field_count * sizeof(struct field_def); > - char *names = (char *)parts + field_count * sizeof(struct key_part_def); > + char *names = (char *)ephemer_key_parts + > + field_count * sizeof(struct key_part_def); > for (uint32_t i = 0; i < field_count; ++i) { > struct field_def *field = &fields[i]; > field->name = names; 2. It is worth extending the commit message with more details, how is it needed for 4256 and 4692, with all what we discussed in private. Please, do the things above and send to Nikita. Also, in case it will be pushed in this form to master, I propose you to create a ticket on ephemeral_space API rework. As you fairly noticed, we have issues with using sql_key_info as, in fact, a format definition. We basically *can't* index fields not added to the format. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format 2020-04-10 19:16 ` Vladislav Shpilevoy @ 2020-04-10 21:52 ` Mergen Imeev 0 siblings, 0 replies; 7+ messages in thread From: Mergen Imeev @ 2020-04-10 21:52 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hi! Thank you for review. My answers below. On Fri, Apr 10, 2020 at 09:16:28PM +0200, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! > > See 2 comments below. > > > diff --git a/src/box/sql.c b/src/box/sql.c > > index 27089bd..288c4fa 100644 > > --- a/src/box/sql.c > > +++ b/src/box/sql.c > > @@ -337,14 +337,15 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > > * length of each name is no more than strlen("_COLUMN_") > > * + 5. > > */ > > - assert(SQL_MAX_COLUMN < 9999); > > + assert(SQL_MAX_COLUMN <= 2000); > > uint32_t name_len = sizeof("_COLUMN_") + 5; > > 1. sizeof("_COLUMN_") != strlen("_COLUMN_"). The latter is 8, > the former is 9. Better use strlen(). > Thanks, fixed. > > uint32_t size = field_count * (sizeof(struct field_def) + name_len + > > sizeof(struct key_part_def)); > > struct field_def *fields = region_alloc(region, size); > > - struct key_part_def *parts = (void *)fields + > > + struct key_part_def *ephemer_key_parts = (void *)fields + > > field_count * sizeof(struct field_def); > > - char *names = (char *)parts + field_count * sizeof(struct key_part_def); > > + char *names = (char *)ephemer_key_parts + > > + field_count * sizeof(struct key_part_def); > > for (uint32_t i = 0; i < field_count; ++i) { > > struct field_def *field = &fields[i]; > > field->name = names; > > 2. It is worth extending the commit message with more details, how > is it needed for 4256 and 4692, with all what we discussed in private. > Done: sql: specify field types in ephemeral space format This patch allows to specify field types in ephemeral space format. Prior to this patch, all fields had a SCALAR field type. This patch allows us to not use the primary index to obtain field types, since now the ephemeral space has field types in the format. This allows us to change the structure of the primary index, which helps to solve the issue #4256. In addition, since we can now set the field types of the ephemeral space, we can use this feature to set the field types according to the left value of the IN operator. This will fix issue #4692. Needed for #4256 Needed for #4692 Closes #3841 > Please, do the things above and send to Nikita. > > Also, in case it will be pushed in this form to master, I propose you to > create a ticket on ephemeral_space API rework. As you fairly noticed, we > have issues with using sql_key_info as, in fact, a format definition. > We basically *can't* index fields not added to the format. Ok, will do. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format @ 2020-04-10 21:54 imeevma 0 siblings, 0 replies; 7+ messages in thread From: imeevma @ 2020-04-10 21:54 UTC (permalink / raw) To: korablev, tsafin, tarantool-patches This patch allows to specify field types in ephemeral space format. Prior to this patch, all fields had a SCALAR field type. This patch allows us to not use the primary index to obtain field types, since now the ephemeral space has field types in the format. This allows us to change the structure of the primary index, which helps to solve the issue #4256. In addition, since we can now set the field types of the ephemeral space, we can use this feature to set the field types according to the left value of the IN operator. This will fix issue #4692. Needed for #4256 Needed for #4692 Closes #3841 --- https://github.com/tarantool/tarantool/issues/3841 https://github.com/tarantool/tarantool/tree/imeevma/gh-3841-format-for-ephemeral-space src/box/space_def.c | 5 ++--- src/box/space_def.h | 3 ++- src/box/sql.c | 52 +++++++++++++++++++++++++++++++++++--------------- src/box/tuple_format.c | 22 ++++++++++----------- 4 files changed, 51 insertions(+), 31 deletions(-) diff --git a/src/box/space_def.c b/src/box/space_def.c index 6611312..5af0f38 100644 --- a/src/box/space_def.c +++ b/src/box/space_def.c @@ -233,7 +233,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, } struct space_def* -space_def_new_ephemeral(uint32_t field_count) +space_def_new_ephemeral(uint32_t field_count, struct field_def *fields) { struct space_opts opts = space_opts_default; opts.is_temporary = true; @@ -242,8 +242,7 @@ space_def_new_ephemeral(uint32_t field_count) "ephemeral", strlen("ephemeral"), "memtx", strlen("memtx"), - &opts, &field_def_default, - 0); + &opts, fields, field_count); return space_def; } diff --git a/src/box/space_def.h b/src/box/space_def.h index ac6d226..61bbdb4 100644 --- a/src/box/space_def.h +++ b/src/box/space_def.h @@ -173,11 +173,12 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, /** * Create a new ephemeral space definition. * @param field_count Number of fields in the space. + * @param fields Field definitions. * * @retval Space definition. */ struct space_def * -space_def_new_ephemeral(uint32_t field_count); +space_def_new_ephemeral(uint32_t field_count, struct field_def *fields); /** * Size of the space_def. diff --git a/src/box/sql.c b/src/box/sql.c index 1256df8..e4434b2 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -330,13 +330,41 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) return NULL; } - struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc, - sizeof(*ephemer_key_parts) * field_count); - if (ephemer_key_parts == NULL) { - diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count, - "region", "key parts"); - return NULL; + struct region *region = &fiber()->gc; + /* + * Name of the fields will be "_COLUMN_1", "_COLUMN_2" + * and so on. Since number of columns no more than 2000, + * length of each name is no more than strlen("_COLUMN_") + * + 5. + */ + assert(SQL_MAX_COLUMN <= 2000); + uint32_t name_len = strlen("_COLUMN_") + 5; + uint32_t size = field_count * (sizeof(struct field_def) + name_len + + sizeof(struct key_part_def)); + struct field_def *fields = region_alloc(region, size); + struct key_part_def *ephemer_key_parts = (void *)fields + + field_count * sizeof(struct field_def); + char *names = (char *)ephemer_key_parts + + field_count * sizeof(struct key_part_def); + for (uint32_t i = 0; i < field_count; ++i) { + struct field_def *field = &fields[i]; + field->name = names; + names += name_len; + sprintf(field->name, "_COLUMN_%d", i); + field->is_nullable = true; + field->nullable_action = ON_CONFLICT_ACTION_NONE; + field->default_value = NULL; + field->default_value_expr = NULL; + if (def != NULL && i < def->part_count) { + assert(def->parts[i].type < field_type_MAX); + field->type = def->parts[i].type; + field->coll_id = def->parts[i].coll_id; + } else { + field->coll_id = COLL_NONE; + field->type = FIELD_TYPE_SCALAR; + } } + for (uint32_t i = 0; i < field_count; ++i) { struct key_part_def *part = &ephemer_key_parts[i]; part->fieldno = i; @@ -344,14 +372,8 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) part->is_nullable = true; part->sort_order = SORT_ORDER_ASC; part->path = NULL; - if (def != NULL && i < def->part_count) { - assert(def->parts[i].type < field_type_MAX); - part->type = def->parts[i].type; - part->coll_id = def->parts[i].coll_id; - } else { - part->coll_id = COLL_NONE; - part->type = FIELD_TYPE_SCALAR; - } + part->type = fields[i].type; + part->coll_id = fields[i].coll_id; } struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts, field_count, false); @@ -370,7 +392,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) rlist_add_entry(&key_list, ephemer_index_def, link); struct space_def *ephemer_space_def = - space_def_new_ephemeral(field_count); + space_def_new_ephemeral(field_count, fields); if (ephemer_space_def == NULL) { index_def_delete(ephemer_index_def); return NULL; diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index 312c966..beaeb0f 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format) * dictionary will never be altered. If it can, then alter can * affect another space, which shares a format with one which is * altered. + * + * The only way to change the format of the space is to recreate + * space with the new format inside of BOX. Since there is no + * mechanism for recreating the ephemeral space, we need not worry + * about changing the format of the ephemeral space. + * * @param p_format Double pointer to format. It is updated with * hashed value, if corresponding format was found * in hash table @@ -709,13 +715,7 @@ static bool tuple_format_reuse(struct tuple_format **p_format) { struct tuple_format *format = *p_format; - if (!format->is_ephemeral) - return false; - /* - * These fields do not participate in hashing. - * Make sure they're unset. - */ - assert(format->dict->name_count == 0); + assert(format->is_ephemeral); assert(format->is_temporary); mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, NULL); @@ -739,9 +739,7 @@ tuple_format_reuse(struct tuple_format **p_format) static int tuple_format_add_to_hash(struct tuple_format *format) { - if(!format->is_ephemeral) - return 0; - assert(format->dict->name_count == 0); + assert(format->is_ephemeral); assert(format->is_temporary); mh_int_t key = mh_tuple_format_put(tuple_formats_hash, (const struct tuple_format **)&format, @@ -795,11 +793,11 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, if (tuple_format_create(format, keys, key_count, space_fields, space_field_count) < 0) goto err; - if (tuple_format_reuse(&format)) + if (is_ephemeral && tuple_format_reuse(&format)) return format; if (tuple_format_register(format) < 0) goto err; - if (tuple_format_add_to_hash(format) < 0) { + if (is_ephemeral && tuple_format_add_to_hash(format) < 0) { tuple_format_deregister(format); goto err; } -- 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-10 21:54 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-02 10:41 [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format imeevma 2020-04-03 13:00 ` Nikita Pettik 2020-04-04 18:34 ` Vladislav Shpilevoy 2020-04-10 0:04 ` Mergen Imeev 2020-04-10 19:16 ` Vladislav Shpilevoy 2020-04-10 21:52 ` Mergen Imeev 2020-04-10 21:54 imeevma
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox