Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v3 0/3] sql: fix order of inserted rows
@ 2020-04-15  7:09 imeevma
  2020-04-15  7:09 ` [Tarantool-patches] [PATCH v3 1/3] box: extend ephemeral space format imeevma
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: imeevma @ 2020-04-15  7:09 UTC (permalink / raw)
  To: korablev, tsafin, tarantool-patches

This patch-set fixes order of inserted rows in case ephemeral
space is used during INSERT into space with sequence. The order
now is the same in which these values were inserted originally.
Wrong order could lead to an error in case NULL was inserted into
field with sequence.

https://github.com/tarantool/tarantool/issues/4256
https://github.com/tarantool/tarantool/tree/imeevma/gh-4256-fix-order-during-insertion

#ChangeLog
 - The inserted values will now be inserted in the order in
   which they were given in case of INSERT into space with
   sesquence (gh-4256).

Changes:
v1:
 - Patch fixed the problem but the logic was too complex.
v2:
 - After exanding of ephemeral space formats, solution become
   simpler.
v3:
 - Expansion of ephemeral space format was divided into two
   patches.

Mergen Imeev (3):
  box: extend ephemeral space format
  sql: specify field types in ephemeral space format
  sql: do not change order of inserted values

 src/box/space_def.c                                | 12 ++--
 src/box/space_def.h                                |  5 +-
 src/box/sql.c                                      | 73 +++++++++++++++++-----
 src/box/sql/insert.c                               | 27 +++++++-
 src/box/sql/select.c                               |  2 +
 src/box/sql/sqlInt.h                               |  2 +
 src/box/sql/vdbe.c                                 | 19 +-----
 src/box/tuple_format.c                             | 22 +++----
 ...256-do-not-change-order-during-insertion.result | 44 +++++++++++++
 ...6-do-not-change-order-during-insertion.test.sql | 13 ++++
 10 files changed, 167 insertions(+), 52 deletions(-)
 create mode 100644 test/sql/gh-4256-do-not-change-order-during-insertion.result
 create mode 100644 test/sql/gh-4256-do-not-change-order-during-insertion.test.sql

-- 
2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Tarantool-patches] [PATCH v3 1/3] box: extend ephemeral space format
  2020-04-15  7:09 [Tarantool-patches] [PATCH v3 0/3] sql: fix order of inserted rows imeevma
@ 2020-04-15  7:09 ` imeevma
  2020-04-15  7:09 ` [Tarantool-patches] [PATCH v3 2/3] sql: specify field types in " imeevma
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: imeevma @ 2020-04-15  7:09 UTC (permalink / raw)
  To: korablev, tsafin, tarantool-patches

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;
 	}

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Tarantool-patches] [PATCH v3 2/3] sql: specify field types in ephemeral space format
  2020-04-15  7:09 [Tarantool-patches] [PATCH v3 0/3] sql: fix order of inserted rows imeevma
  2020-04-15  7:09 ` [Tarantool-patches] [PATCH v3 1/3] box: extend ephemeral space format imeevma
@ 2020-04-15  7:09 ` imeevma
  2020-04-15  7:09 ` [Tarantool-patches] [PATCH v3 3/3] sql: do not change order of inserted values imeevma
  2020-04-16 13:29 ` [Tarantool-patches] [PATCH v3 0/3] sql: fix order of inserted rows Nikita Pettik
  3 siblings, 0 replies; 7+ messages in thread
From: imeevma @ 2020-04-15  7:09 UTC (permalink / raw)
  To: korablev, tsafin, tarantool-patches

This patch specifies field types in ephemeral space format in SQL.
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
---
 src/box/sql.c | 53 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index f6afb86..6436f64 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -324,13 +324,44 @@ 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");
+	struct region *region = &fiber()->gc;
+	/*
+	 * Name of the fields will be "_COLUMN_1", "_COLUMN_2"
+	 * and so on. Due to this, length of each name is no more
+	 * than strlen("_COLUMN_") plus length of UINT32_MAX
+	 * turned to string, which is 10 and plus 1 for \0.
+	 */
+	uint32_t name_len = strlen("_COLUMN_") + 11;
+	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
+				       sizeof(struct key_part_def));
+	struct field_def *fields = region_alloc(region, size);
+	if (fields == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc", "fields");
 		return NULL;
 	}
+	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;
@@ -338,14 +369,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);
@@ -364,7 +389,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, NULL);
+		space_def_new_ephemeral(field_count, fields);
 	if (ephemer_space_def == NULL) {
 		index_def_delete(ephemer_index_def);
 		return NULL;
-- 
2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Tarantool-patches] [PATCH v3 3/3] sql: do not change order of inserted values
  2020-04-15  7:09 [Tarantool-patches] [PATCH v3 0/3] sql: fix order of inserted rows imeevma
  2020-04-15  7:09 ` [Tarantool-patches] [PATCH v3 1/3] box: extend ephemeral space format imeevma
  2020-04-15  7:09 ` [Tarantool-patches] [PATCH v3 2/3] sql: specify field types in " imeevma
@ 2020-04-15  7:09 ` imeevma
  2020-04-16  0:09   ` Nikita Pettik
  2020-04-16 13:29 ` [Tarantool-patches] [PATCH v3 0/3] sql: fix order of inserted rows Nikita Pettik
  3 siblings, 1 reply; 7+ messages in thread
From: imeevma @ 2020-04-15  7:09 UTC (permalink / raw)
  To: korablev, tsafin, tarantool-patches

My answers and new patch below.

On 4/14/20 1:59 AM, Nikita Pettik wrote:
> On 12 Apr 19:29, imeevma@tarantool.org wrote:
>> @@ -365,18 +372,21 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
>>  		}
>>  	}
>>  
>> -	for (uint32_t i = 0; i < field_count; ++i) {
>> +	for (uint32_t i = 0; i < part_count; ++i) {
>>  		struct key_part_def *part = &ephemer_key_parts[i];
>> -		part->fieldno = i;
>
> I'd place comment explaining value of j and position of 'rowid'.
>
Fixed.

>> +		uint32_t j = i;
>> +		if (key_info != NULL && key_info->is_pk_rowid)
>> +			j = field_count - 1;
>> +		part->fieldno = j;
>>  		part->nullable_action = ON_CONFLICT_ACTION_NONE;
>>  		part->is_nullable = true;
>>  		part->sort_order = SORT_ORDER_ASC;
>>  		part->path = NULL;
>> -		part->type = fields[i].type;
>> -		part->coll_id = fields[i].coll_id;
>> +		part->type = fields[j].type;
>> +		part->coll_id = fields[j].coll_id;
>>  	}
>>  	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
>> -						      field_count, false);
>> +						      part_count, false);
>>  	if (ephemer_key_def == NULL)
>>  		return NULL;
>>  
>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index 43a0de5..52a948d 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> @@ -455,8 +455,23 @@ sqlInsert(Parse * pParse,	/* Parser context */
>>  			reg_eph = ++pParse->nMem;
>>  			regRec = sqlGetTempReg(pParse);
>>  			regCopy = sqlGetTempRange(pParse, nColumn + 1);
>> -			sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
>> -					  nColumn + 1);
>> +			/*
>> +			 * This key_info is used to show that
>> +			 * rowid should be the first part of PK.
>> +			 * This way we will save initial order of
>> +			 * the inserted values. The order is
>> +			 * important if we use the AUTOINCREMENT
>> +			 * feature, since changing the order can
>> +			 * change the number inserted instead of
>> +			 * NULL.
>> +			 */
>
> So why don't you use this trick only if space with autoincrement
> is affected? So that proceed with old way for all other spaces.
>
Fixed.

>> +			struct sql_key_info *key_info =
>> +				sql_key_info_new(pParse->db, nColumn + 1);
>> +			key_info->parts[nColumn].type = FIELD_TYPE_UNSIGNED;
>> +			key_info->is_pk_rowid = true;
>> +			sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph,
>> +				      nColumn + 1, 0, (char *)key_info,
>> +				      P4_KEYINFO);
>>  			addrL = sqlVdbeAddOp1(v, OP_Yield, dest.iSDParm);
>>  			VdbeCoverage(v);
>>  			sqlVdbeAddOp2(v, OP_NextIdEphemeral, reg_eph,
>> diff --git a/test/sql/gh-4256-do-not-change-order-during-insertion.test.lua b/test/sql/gh-4256-do-not-change-order-during-insertion.test.lua
>> new file mode 100644
>> index 0000000..4d8367c
>> --- /dev/null
>> +++ b/test/sql/gh-4256-do-not-change-order-during-insertion.test.lua
>> @@ -0,0 +1,15 @@
>
> Btw, why don't you use SQL format of tests?
>
Fixed.

>> +test_run = require('test_run').new()
>> +engine = test_run:get_cfg('engine')
>> +--
>> +-- Make sure that when inserting, values are inserted in the given
>> +-- order when ephemeral space is used.
>> +--
>> +box.execute('CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT);')
>> +--
>> +-- In order for this INSERT to use the ephemeral space, we created
>> +-- this trigger.
>> +--
>> +box.execute('CREATE TRIGGER r AFTER INSERT ON t FOR EACH ROW BEGIN SELECT 1; END')
>> +box.execute('INSERT INTO t VALUES (1), (NULL), (10), (NULL), (NULL), (3), (NULL);')
>> +box.execute('SELECT * FROM t;')
>> +box.execute('DROP TABLE t;')
>> -- 
>> 2.7.4
>>


New patch:

From 2542c80348de25f26cac73e101b394a1873156da Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Thu, 2 Apr 2020 11:15:24 +0300
Subject: [PATCH] sql: do not change order of inserted values

Before this patch, if an ephemeral space was used during INSERT or
REPLACE, the inserted values were sorted by the first column,
since this was the first part of the index. This can lead to an
error when using the AUTOINCREMENT feature, since changing the
order of the inserted value can change the value inserted instead
of NULL. To avoid this, the patch makes the rowid of the inserted
row in the ephemeral space the only part of the ephemeral space
index.

Closes #4256

diff --git a/src/box/sql.c b/src/box/sql.c
index 6436f64..fc1386f 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -318,10 +318,17 @@ struct space *
 sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 {
 	struct key_def *def = NULL;
+	uint32_t part_count = field_count;
 	if (key_info != NULL) {
 		def = sql_key_info_to_key_def(key_info);
 		if (def == NULL)
 			return NULL;
+		/*
+		 * In case is_pk_rowid is true we can use rowid
+		 * as the only part of the key.
+		 */
+		if (key_info->is_pk_rowid)
+			part_count = 1;
 	}
 
 	struct region *region = &fiber()->gc;
@@ -332,8 +339,8 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	 * turned to string, which is 10 and plus 1 for \0.
 	 */
 	uint32_t name_len = strlen("_COLUMN_") + 11;
-	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
-				       sizeof(struct key_part_def));
+	uint32_t size = field_count * (sizeof(struct field_def) + name_len) +
+			part_count * sizeof(struct key_part_def);
 	struct field_def *fields = region_alloc(region, size);
 	if (fields == NULL) {
 		diag_set(OutOfMemory, size, "region_alloc", "fields");
@@ -342,7 +349,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	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);
+		       part_count * sizeof(struct key_part_def);
 	for (uint32_t i = 0; i < field_count; ++i) {
 		struct field_def *field = &fields[i];
 		field->name = names;
@@ -362,18 +369,27 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 		}
 	}
 
-	for (uint32_t i = 0; i < field_count; ++i) {
+	for (uint32_t i = 0; i < part_count; ++i) {
 		struct key_part_def *part = &ephemer_key_parts[i];
-		part->fieldno = i;
+		/*
+		 * In case we need to save initial order of
+		 * inserted into ephemeral space rows we use rowid
+		 * as the only part of PK. If ephemeral space has
+		 * a rowid, it is always the last column.
+		 */
+		uint32_t j = i;
+		if (key_info != NULL && key_info->is_pk_rowid)
+			j = field_count - 1;
+		part->fieldno = j;
 		part->nullable_action = ON_CONFLICT_ACTION_NONE;
 		part->is_nullable = true;
 		part->sort_order = SORT_ORDER_ASC;
 		part->path = NULL;
-		part->type = fields[i].type;
-		part->coll_id = fields[i].coll_id;
+		part->type = fields[j].type;
+		part->coll_id = fields[j].coll_id;
 	}
 	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
-						      field_count, false);
+						      part_count, false);
 	if (ephemer_key_def == NULL)
 		return NULL;
 
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 43a0de5..787855e 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -455,8 +455,31 @@ sqlInsert(Parse * pParse,	/* Parser context */
 			reg_eph = ++pParse->nMem;
 			regRec = sqlGetTempReg(pParse);
 			regCopy = sqlGetTempRange(pParse, nColumn + 1);
-			sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
-					  nColumn + 1);
+			/*
+			 * This key_info is used to show that
+			 * rowid should be the first part of PK in
+			 * case we used AUTOINCREMENT feature.
+			 * This way we will save initial order of
+			 * the inserted values. The order is
+			 * important if we use the AUTOINCREMENT
+			 * feature, since changing the order can
+			 * change the number inserted instead of
+			 * NULL.
+			 */
+			if (space->sequence != NULL) {
+				struct sql_key_info *key_info =
+					sql_key_info_new(pParse->db,
+							 nColumn + 1);
+				key_info->parts[nColumn].type =
+					FIELD_TYPE_UNSIGNED;
+				key_info->is_pk_rowid = true;
+				sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph,
+					      nColumn + 1, 0, (char *)key_info,
+					      P4_KEYINFO);
+			} else {
+				sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
+					      nColumn + 1);
+			}
 			addrL = sqlVdbeAddOp1(v, OP_Yield, dest.iSDParm);
 			VdbeCoverage(v);
 			sqlVdbeAddOp2(v, OP_NextIdEphemeral, reg_eph,
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 65e41f2..f394840 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1422,6 +1422,7 @@ sql_key_info_new(sql *db, uint32_t part_count)
 	key_info->key_def = NULL;
 	key_info->refs = 1;
 	key_info->part_count = part_count;
+	key_info->is_pk_rowid = false;
 	for (uint32_t i = 0; i < part_count; i++) {
 		struct key_part_def *part = &key_info->parts[i];
 		part->fieldno = i;
@@ -1448,6 +1449,7 @@ sql_key_info_new_from_key_def(sql *db, const struct key_def *key_def)
 	key_info->key_def = NULL;
 	key_info->refs = 1;
 	key_info->part_count = key_def->part_count;
+	key_info->is_pk_rowid = false;
 	key_def_dump_parts(key_def, key_info->parts, NULL);
 	return key_info;
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index e45a767..37283e5 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4118,6 +4118,8 @@ struct sql_key_info {
 	struct key_def *key_def;
 	/** Reference counter. */
 	uint32_t refs;
+	/** Rowid should be the only part of PK, if true. */
+	bool is_pk_rowid;
 	/** Number of parts in the key. */
 	uint32_t part_count;
 	/** Definition of the key parts. */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 738448e..a7549c2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2695,23 +2695,10 @@ case OP_Column: {
 		pC->cacheStatus = p->cacheCtr;
 	}
 	enum field_type field_type = field_type_MAX;
-	if (pC->eCurType == CURTYPE_TARANTOOL) {
-		/*
-		 * Ephemeral spaces feature only one index
-		 * covering all fields, but ephemeral spaces
-		 * lack format. So, we can fetch type from
-		 * key parts.
-		 */
-		if (pC->uc.pCursor->curFlags & BTCF_TEphemCursor) {
-			field_type = pC->uc.pCursor->index->def->
-					key_def->parts[p2].type;
-		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
-			field_type = pC->uc.pCursor->space->def->
-					fields[p2].type;
-		}
-	} else if (pC->eCurType == CURTYPE_SORTER) {
+	if (pC->eCurType == CURTYPE_TARANTOOL)
+		field_type = pC->uc.pCursor->space->def->fields[p2].type;
+	else if (pC->eCurType == CURTYPE_SORTER)
 		field_type = vdbe_sorter_get_field_type(pC->uc.pSorter, p2);
-	}
 	struct Mem *default_val_mem =
 		pOp->p4type == P4_MEM ? pOp->p4.pMem : NULL;
 	if (vdbe_field_ref_fetch(&pC->field_ref, p2, pDest) != 0)
diff --git a/test/sql/gh-4256-do-not-change-order-during-insertion.result b/test/sql/gh-4256-do-not-change-order-during-insertion.result
new file mode 100644
index 0000000..8a585b3
--- /dev/null
+++ b/test/sql/gh-4256-do-not-change-order-during-insertion.result
@@ -0,0 +1,44 @@
+-- test-run result file version 2
+--
+-- Make sure that when inserting, values are inserted in the given
+-- order when ephemeral space is used.
+--
+CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT);
+ | ---
+ | - row_count: 1
+ | ...
+--
+-- In order for this INSERT to use the ephemeral space, we created
+-- this trigger.
+--
+CREATE TRIGGER r AFTER INSERT ON t FOR EACH ROW BEGIN SELECT 1; END
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO t VALUES (1), (NULL), (10), (NULL), (NULL), (3), (NULL);
+ | ---
+ | - autoincrement_ids:
+ |   - 2
+ |   - 11
+ |   - 12
+ |   - 13
+ |   row_count: 7
+ | ...
+SELECT * FROM t;
+ | ---
+ | - metadata:
+ |   - name: I
+ |     type: integer
+ |   rows:
+ |   - [1]
+ |   - [2]
+ |   - [3]
+ |   - [10]
+ |   - [11]
+ |   - [12]
+ |   - [13]
+ | ...
+DROP TABLE t;
+ | ---
+ | - row_count: 1
+ | ...
diff --git a/test/sql/gh-4256-do-not-change-order-during-insertion.test.sql b/test/sql/gh-4256-do-not-change-order-during-insertion.test.sql
new file mode 100644
index 0000000..9b5afa0
--- /dev/null
+++ b/test/sql/gh-4256-do-not-change-order-during-insertion.test.sql
@@ -0,0 +1,13 @@
+--
+-- Make sure that when inserting, values are inserted in the given
+-- order when ephemeral space is used.
+--
+CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT);
+--
+-- In order for this INSERT to use the ephemeral space, we created
+-- this trigger.
+--
+CREATE TRIGGER r AFTER INSERT ON t FOR EACH ROW BEGIN SELECT 1; END
+INSERT INTO t VALUES (1), (NULL), (10), (NULL), (NULL), (3), (NULL);
+SELECT * FROM t;
+DROP TABLE t;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 3/3] sql: do not change order of inserted values
  2020-04-15  7:09 ` [Tarantool-patches] [PATCH v3 3/3] sql: do not change order of inserted values imeevma
@ 2020-04-16  0:09   ` Nikita Pettik
  2020-04-16  8:33     ` Mergen Imeev
  0 siblings, 1 reply; 7+ messages in thread
From: Nikita Pettik @ 2020-04-16  0:09 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

On 15 Apr 10:09, imeevma@tarantool.org wrote:
> My answers and new patch below.
> 
> On 4/14/20 1:59 AM, Nikita Pettik wrote:
>  
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 43a0de5..787855e 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -455,8 +455,31 @@ sqlInsert(Parse * pParse,	/* Parser context */
>  			reg_eph = ++pParse->nMem;
>  			regRec = sqlGetTempReg(pParse);
>  			regCopy = sqlGetTempRange(pParse, nColumn + 1);
> -			sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
> -					  nColumn + 1);
> +			/*
> +			 * This key_info is used to show that
> +			 * rowid should be the first part of PK in
> +			 * case we used AUTOINCREMENT feature.
> +			 * This way we will save initial order of
> +			 * the inserted values. The order is
> +			 * important if we use the AUTOINCREMENT
> +			 * feature, since changing the order can
> +			 * change the number inserted instead of
> +			 * NULL.
> +			 */
> +			if (space->sequence != NULL) {
> +				struct sql_key_info *key_info =
> +					sql_key_info_new(pParse->db,
> +							 nColumn + 1);
> +				key_info->parts[nColumn].type =
> +					FIELD_TYPE_UNSIGNED;
> +				key_info->is_pk_rowid = true;
> +				sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph,
> +					      nColumn + 1, 0, (char *)key_info,
> +					      P4_KEYINFO);
> +			} else {
> +				sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
> +					      nColumn + 1);
> +			}

You can remove 'else' branch and always add OP_OPenTEphemeral with
2 operands. Then inside 'if' branch invoke sqlVdbeChangeP4().
Otherwise LGTM.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 3/3] sql: do not change order of inserted values
  2020-04-16  0:09   ` Nikita Pettik
@ 2020-04-16  8:33     ` Mergen Imeev
  0 siblings, 0 replies; 7+ messages in thread
From: Mergen Imeev @ 2020-04-16  8:33 UTC (permalink / raw)
  To: Nikita Pettik; +Cc: tarantool-patches

Thank you for review! My answer, diff and new patch below.

On Thu, Apr 16, 2020 at 12:09:57AM +0000, Nikita Pettik wrote:
> On 15 Apr 10:09, imeevma@tarantool.org wrote:
> > My answers and new patch below.
> > 
> > On 4/14/20 1:59 AM, Nikita Pettik wrote:
> >  
> > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> > index 43a0de5..787855e 100644
> > --- a/src/box/sql/insert.c
> > +++ b/src/box/sql/insert.c
> > @@ -455,8 +455,31 @@ sqlInsert(Parse * pParse,	/* Parser context */
> >  			reg_eph = ++pParse->nMem;
> >  			regRec = sqlGetTempReg(pParse);
> >  			regCopy = sqlGetTempRange(pParse, nColumn + 1);
> > -			sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
> > -					  nColumn + 1);
> > +			/*
> > +			 * This key_info is used to show that
> > +			 * rowid should be the first part of PK in
> > +			 * case we used AUTOINCREMENT feature.
> > +			 * This way we will save initial order of
> > +			 * the inserted values. The order is
> > +			 * important if we use the AUTOINCREMENT
> > +			 * feature, since changing the order can
> > +			 * change the number inserted instead of
> > +			 * NULL.
> > +			 */
> > +			if (space->sequence != NULL) {
> > +				struct sql_key_info *key_info =
> > +					sql_key_info_new(pParse->db,
> > +							 nColumn + 1);
> > +				key_info->parts[nColumn].type =
> > +					FIELD_TYPE_UNSIGNED;
> > +				key_info->is_pk_rowid = true;
> > +				sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph,
> > +					      nColumn + 1, 0, (char *)key_info,
> > +					      P4_KEYINFO);
> > +			} else {
> > +				sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
> > +					      nColumn + 1);
> > +			}
> 
> You can remove 'else' branch and always add OP_OPenTEphemeral with
> 2 operands. Then inside 'if' branch invoke sqlVdbeChangeP4().
> Otherwise LGTM.
> 
Fixed.

Diff:


diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 787855e..588e142 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -455,6 +455,8 @@ sqlInsert(Parse * pParse,	/* Parser context */
 			reg_eph = ++pParse->nMem;
 			regRec = sqlGetTempReg(pParse);
 			regCopy = sqlGetTempRange(pParse, nColumn + 1);
+			sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
+					  nColumn + 1);
 			/*
 			 * This key_info is used to show that
 			 * rowid should be the first part of PK in
@@ -473,12 +475,8 @@ sqlInsert(Parse * pParse,	/* Parser context */
 				key_info->parts[nColumn].type =
 					FIELD_TYPE_UNSIGNED;
 				key_info->is_pk_rowid = true;
-				sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph,
-					      nColumn + 1, 0, (char *)key_info,
-					      P4_KEYINFO);
-			} else {
-				sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
-					      nColumn + 1);
+				sqlVdbeChangeP4(v, -1, (void *)key_info,
+					        P4_KEYINFO);
 			}
 			addrL = sqlVdbeAddOp1(v, OP_Yield, dest.iSDParm);
 			VdbeCoverage(v);

New patch:


From 167e3e17005b859d0fb2fc9ae09d5a251f54f9f0 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Thu, 2 Apr 2020 11:15:24 +0300
Subject: [PATCH] sql: do not change order of inserted values

Before this patch, if an ephemeral space was used during INSERT or
REPLACE, the inserted values were sorted by the first column,
since this was the first part of the index. This can lead to an
error when using the AUTOINCREMENT feature, since changing the
order of the inserted value can change the value inserted instead
of NULL. To avoid this, the patch makes the rowid of the inserted
row in the ephemeral space the only part of the ephemeral space
index.

Closes #4256

diff --git a/src/box/sql.c b/src/box/sql.c
index 6436f64..fc1386f 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -318,10 +318,17 @@ struct space *
 sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 {
 	struct key_def *def = NULL;
+	uint32_t part_count = field_count;
 	if (key_info != NULL) {
 		def = sql_key_info_to_key_def(key_info);
 		if (def == NULL)
 			return NULL;
+		/*
+		 * In case is_pk_rowid is true we can use rowid
+		 * as the only part of the key.
+		 */
+		if (key_info->is_pk_rowid)
+			part_count = 1;
 	}
 
 	struct region *region = &fiber()->gc;
@@ -332,8 +339,8 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	 * turned to string, which is 10 and plus 1 for \0.
 	 */
 	uint32_t name_len = strlen("_COLUMN_") + 11;
-	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
-				       sizeof(struct key_part_def));
+	uint32_t size = field_count * (sizeof(struct field_def) + name_len) +
+			part_count * sizeof(struct key_part_def);
 	struct field_def *fields = region_alloc(region, size);
 	if (fields == NULL) {
 		diag_set(OutOfMemory, size, "region_alloc", "fields");
@@ -342,7 +349,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	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);
+		       part_count * sizeof(struct key_part_def);
 	for (uint32_t i = 0; i < field_count; ++i) {
 		struct field_def *field = &fields[i];
 		field->name = names;
@@ -362,18 +369,27 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 		}
 	}
 
-	for (uint32_t i = 0; i < field_count; ++i) {
+	for (uint32_t i = 0; i < part_count; ++i) {
 		struct key_part_def *part = &ephemer_key_parts[i];
-		part->fieldno = i;
+		/*
+		 * In case we need to save initial order of
+		 * inserted into ephemeral space rows we use rowid
+		 * as the only part of PK. If ephemeral space has
+		 * a rowid, it is always the last column.
+		 */
+		uint32_t j = i;
+		if (key_info != NULL && key_info->is_pk_rowid)
+			j = field_count - 1;
+		part->fieldno = j;
 		part->nullable_action = ON_CONFLICT_ACTION_NONE;
 		part->is_nullable = true;
 		part->sort_order = SORT_ORDER_ASC;
 		part->path = NULL;
-		part->type = fields[i].type;
-		part->coll_id = fields[i].coll_id;
+		part->type = fields[j].type;
+		part->coll_id = fields[j].coll_id;
 	}
 	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
-						      field_count, false);
+						      part_count, false);
 	if (ephemer_key_def == NULL)
 		return NULL;
 
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 43a0de5..588e142 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -457,6 +457,27 @@ sqlInsert(Parse * pParse,	/* Parser context */
 			regCopy = sqlGetTempRange(pParse, nColumn + 1);
 			sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
 					  nColumn + 1);
+			/*
+			 * This key_info is used to show that
+			 * rowid should be the first part of PK in
+			 * case we used AUTOINCREMENT feature.
+			 * This way we will save initial order of
+			 * the inserted values. The order is
+			 * important if we use the AUTOINCREMENT
+			 * feature, since changing the order can
+			 * change the number inserted instead of
+			 * NULL.
+			 */
+			if (space->sequence != NULL) {
+				struct sql_key_info *key_info =
+					sql_key_info_new(pParse->db,
+							 nColumn + 1);
+				key_info->parts[nColumn].type =
+					FIELD_TYPE_UNSIGNED;
+				key_info->is_pk_rowid = true;
+				sqlVdbeChangeP4(v, -1, (void *)key_info,
+					        P4_KEYINFO);
+			}
 			addrL = sqlVdbeAddOp1(v, OP_Yield, dest.iSDParm);
 			VdbeCoverage(v);
 			sqlVdbeAddOp2(v, OP_NextIdEphemeral, reg_eph,
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 65e41f2..f394840 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1422,6 +1422,7 @@ sql_key_info_new(sql *db, uint32_t part_count)
 	key_info->key_def = NULL;
 	key_info->refs = 1;
 	key_info->part_count = part_count;
+	key_info->is_pk_rowid = false;
 	for (uint32_t i = 0; i < part_count; i++) {
 		struct key_part_def *part = &key_info->parts[i];
 		part->fieldno = i;
@@ -1448,6 +1449,7 @@ sql_key_info_new_from_key_def(sql *db, const struct key_def *key_def)
 	key_info->key_def = NULL;
 	key_info->refs = 1;
 	key_info->part_count = key_def->part_count;
+	key_info->is_pk_rowid = false;
 	key_def_dump_parts(key_def, key_info->parts, NULL);
 	return key_info;
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index e45a767..37283e5 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4118,6 +4118,8 @@ struct sql_key_info {
 	struct key_def *key_def;
 	/** Reference counter. */
 	uint32_t refs;
+	/** Rowid should be the only part of PK, if true. */
+	bool is_pk_rowid;
 	/** Number of parts in the key. */
 	uint32_t part_count;
 	/** Definition of the key parts. */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 5ab9779..724bc18 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2702,23 +2702,10 @@ case OP_Column: {
 		pC->cacheStatus = p->cacheCtr;
 	}
 	enum field_type field_type = field_type_MAX;
-	if (pC->eCurType == CURTYPE_TARANTOOL) {
-		/*
-		 * Ephemeral spaces feature only one index
-		 * covering all fields, but ephemeral spaces
-		 * lack format. So, we can fetch type from
-		 * key parts.
-		 */
-		if (pC->uc.pCursor->curFlags & BTCF_TEphemCursor) {
-			field_type = pC->uc.pCursor->index->def->
-					key_def->parts[p2].type;
-		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
-			field_type = pC->uc.pCursor->space->def->
-					fields[p2].type;
-		}
-	} else if (pC->eCurType == CURTYPE_SORTER) {
+	if (pC->eCurType == CURTYPE_TARANTOOL)
+		field_type = pC->uc.pCursor->space->def->fields[p2].type;
+	else if (pC->eCurType == CURTYPE_SORTER)
 		field_type = vdbe_sorter_get_field_type(pC->uc.pSorter, p2);
-	}
 	struct Mem *default_val_mem =
 		pOp->p4type == P4_MEM ? pOp->p4.pMem : NULL;
 	if (vdbe_field_ref_fetch(&pC->field_ref, p2, pDest) != 0)
diff --git a/test/sql/gh-4256-do-not-change-order-during-insertion.result b/test/sql/gh-4256-do-not-change-order-during-insertion.result
new file mode 100644
index 0000000..8a585b3
--- /dev/null
+++ b/test/sql/gh-4256-do-not-change-order-during-insertion.result
@@ -0,0 +1,44 @@
+-- test-run result file version 2
+--
+-- Make sure that when inserting, values are inserted in the given
+-- order when ephemeral space is used.
+--
+CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT);
+ | ---
+ | - row_count: 1
+ | ...
+--
+-- In order for this INSERT to use the ephemeral space, we created
+-- this trigger.
+--
+CREATE TRIGGER r AFTER INSERT ON t FOR EACH ROW BEGIN SELECT 1; END
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO t VALUES (1), (NULL), (10), (NULL), (NULL), (3), (NULL);
+ | ---
+ | - autoincrement_ids:
+ |   - 2
+ |   - 11
+ |   - 12
+ |   - 13
+ |   row_count: 7
+ | ...
+SELECT * FROM t;
+ | ---
+ | - metadata:
+ |   - name: I
+ |     type: integer
+ |   rows:
+ |   - [1]
+ |   - [2]
+ |   - [3]
+ |   - [10]
+ |   - [11]
+ |   - [12]
+ |   - [13]
+ | ...
+DROP TABLE t;
+ | ---
+ | - row_count: 1
+ | ...
diff --git a/test/sql/gh-4256-do-not-change-order-during-insertion.test.sql b/test/sql/gh-4256-do-not-change-order-during-insertion.test.sql
new file mode 100644
index 0000000..9b5afa0
--- /dev/null
+++ b/test/sql/gh-4256-do-not-change-order-during-insertion.test.sql
@@ -0,0 +1,13 @@
+--
+-- Make sure that when inserting, values are inserted in the given
+-- order when ephemeral space is used.
+--
+CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT);
+--
+-- In order for this INSERT to use the ephemeral space, we created
+-- this trigger.
+--
+CREATE TRIGGER r AFTER INSERT ON t FOR EACH ROW BEGIN SELECT 1; END
+INSERT INTO t VALUES (1), (NULL), (10), (NULL), (NULL), (3), (NULL);
+SELECT * FROM t;
+DROP TABLE t;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 0/3] sql: fix order of inserted rows
  2020-04-15  7:09 [Tarantool-patches] [PATCH v3 0/3] sql: fix order of inserted rows imeevma
                   ` (2 preceding siblings ...)
  2020-04-15  7:09 ` [Tarantool-patches] [PATCH v3 3/3] sql: do not change order of inserted values imeevma
@ 2020-04-16 13:29 ` Nikita Pettik
  3 siblings, 0 replies; 7+ messages in thread
From: Nikita Pettik @ 2020-04-16 13:29 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

On 15 Apr 10:09, imeevma@tarantool.org wrote:
> This patch-set fixes order of inserted rows in case ephemeral
> space is used during INSERT into space with sequence. The order
> now is the same in which these values were inserted originally.
> Wrong order could lead to an error in case NULL was inserted into
> field with sequence.
> 
> https://github.com/tarantool/tarantool/issues/4256
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4256-fix-order-during-insertion

Pushed to master, backported to 2.3 and 2.2. Changelogs are updated
correspondingly. Branch is dropped.

> #ChangeLog
>  - The inserted values will now be inserted in the order in
>    which they were given in case of INSERT into space with
>    sesquence (gh-4256).
> 
> Changes:
> v1:
>  - Patch fixed the problem but the logic was too complex.
> v2:
>  - After exanding of ephemeral space formats, solution become
>    simpler.

Please, check out proper format of changelogs between versions:
https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/#how-to-submit-a-patch-for-review

> v3:
>  - Expansion of ephemeral space format was divided into two
>    patches.
> 
> Mergen Imeev (3):
>   box: extend ephemeral space format
>   sql: specify field types in ephemeral space format
>   sql: do not change order of inserted values
> 
>  src/box/space_def.c                                | 12 ++--
>  src/box/space_def.h                                |  5 +-
>  src/box/sql.c                                      | 73 +++++++++++++++++-----
>  src/box/sql/insert.c                               | 27 +++++++-
>  src/box/sql/select.c                               |  2 +
>  src/box/sql/sqlInt.h                               |  2 +
>  src/box/sql/vdbe.c                                 | 19 +-----
>  src/box/tuple_format.c                             | 22 +++----
>  ...256-do-not-change-order-during-insertion.result | 44 +++++++++++++
>  ...6-do-not-change-order-during-insertion.test.sql | 13 ++++
>  10 files changed, 167 insertions(+), 52 deletions(-)
>  create mode 100644 test/sql/gh-4256-do-not-change-order-during-insertion.result
>  create mode 100644 test/sql/gh-4256-do-not-change-order-during-insertion.test.sql
> 
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-04-16 13:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-15  7:09 [Tarantool-patches] [PATCH v3 0/3] sql: fix order of inserted rows imeevma
2020-04-15  7:09 ` [Tarantool-patches] [PATCH v3 1/3] box: extend ephemeral space format imeevma
2020-04-15  7:09 ` [Tarantool-patches] [PATCH v3 2/3] sql: specify field types in " imeevma
2020-04-15  7:09 ` [Tarantool-patches] [PATCH v3 3/3] sql: do not change order of inserted values imeevma
2020-04-16  0:09   ` Nikita Pettik
2020-04-16  8:33     ` Mergen Imeev
2020-04-16 13:29 ` [Tarantool-patches] [PATCH v3 0/3] sql: fix order of inserted rows Nikita Pettik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox