[Tarantool-patches] [PATCH v3 3/3] sql: do not change order of inserted values

[imeevma@tarantool.org]

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;