[Tarantool-patches] [PATCH v3 1/3] box: extend ephemeral space format

[imeevma@tarantool.org]

Hi! Thank you for the review. My answers and new patch below.

On 4/14/20 1:47 AM, Nikita Pettik wrote:
> On 12 Apr 19:29, imeevma@tarantool.org wrote:
>> 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);
>
> Ephemeral space is capable of storing more columns than casual table.
> For instance eph. table holding result of join operations features
> number of columns which equals to sum of all table's columns
> participating in join.
>
Fixed. Now insted of 4 figures, we may use 10.

>> +	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);
>
> NULL check?
>
Fixed.

>> +	struct key_part_def *ephemer_key_parts = (void *)fields +
>> +				     field_count * sizeof(struct field_def);
>
> Strange indentation.
>
Fixed.

>> +	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;
>> +		}
>>  	}
>> 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);
>
> Personally I'd split this patch into two: one which affects box
> component, and another one which introduces format of ephemeral
> spaces in SQL.
>
Fixed.

New patch:

From 7bd2ab4f48e1cdbd31bb273252849782b45e2df3 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Tue, 14 Apr 2020 23:37:29 +0300
Subject: [PATCH] box: extend ephemeral space format

This patch allows to set field types and names in ephemeral space
formats.

Needed for #4256
Needed for #4692
Part of #3841

diff --git a/src/box/space_def.c b/src/box/space_def.c
index 6611312..0ff51b9 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -233,17 +233,21 @@ 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 exact_field_count, struct field_def *fields)
 {
 	struct space_opts opts = space_opts_default;
 	opts.is_temporary = true;
 	opts.is_ephemeral = true;
-	struct space_def *space_def = space_def_new(0, 0, field_count,
+	uint32_t field_count = exact_field_count;
+	if (fields == NULL) {
+		fields = (struct field_def *)&field_def_default;
+		field_count = 0;
+	}
+	struct space_def *space_def = space_def_new(0, 0, exact_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..788b601 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -172,12 +172,13 @@ 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 exact_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 exact_field_count, struct field_def *fields);
 
 /**
  * Size of the space_def.
diff --git a/src/box/sql.c b/src/box/sql.c
index 6afb308..f6afb86 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -364,7 +364,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, NULL);
 	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;
 	}