[Tarantool-patches] [PATCH v1 1/1] sql: use rowid as PK of ephemeral space on INSERT

[imeevma@tarantool.org]

Prior to this patch, the primary key of the ephemeral space always
consisted of all columns of the ephemeral space. This can lead to
an error during insertion when using ephemeral space. The problem
was that after selecting from the ephemeral space, the values were
sorted differently than when they were initially inserted.

This patch fixes sorting in ephemeral space in case it was used in
INSERT.

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

 src/box/sql.c                   | 54 +++++++++++++++++++++++++++--------------
 src/box/sql/insert.c            |  4 +--
 src/box/sql/tarantoolInt.h      |  4 ++-
 src/box/sql/vdbe.c              | 23 ++++++++++++++----
 test/sql/autoincrement.result   | 40 ++++++++++++++++++++++++++++++
 test/sql/autoincrement.test.lua | 10 ++++++++
 6 files changed, 109 insertions(+), 26 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df8..1e9290c 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -321,8 +321,10 @@ tarantoolsqlCount(struct BtCursor *pCur)
 }
 
 struct space *
-sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
+sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info,
+			   uint32_t rowid)
 {
+	assert(rowid == 0 || key_info == NULL);
 	struct key_def *def = NULL;
 	if (key_info != NULL) {
 		def = sql_key_info_to_key_def(key_info);
@@ -330,31 +332,47 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 			return NULL;
 	}
 
+	uint32_t index_size;
+	if (rowid == 0)
+		index_size = field_count;
+	else
+		index_size = 1;
+
 	struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc,
-				sizeof(*ephemer_key_parts) * field_count);
+				sizeof(*ephemer_key_parts) * index_size);
 	if (ephemer_key_parts == NULL) {
-		diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count,
+		diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * index_size,
 			 "region", "key parts");
 		return NULL;
 	}
-	for (uint32_t i = 0; i < field_count; ++i) {
-		struct key_part_def *part = &ephemer_key_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;
+	if (rowid == 0) {
+		for (uint32_t i = 0; i < field_count; ++i) {
+			struct key_part_def *part = &ephemer_key_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;
+			}
 		}
+	} else {
+		ephemer_key_parts->fieldno = rowid;
+		ephemer_key_parts->nullable_action = ON_CONFLICT_ACTION_ABORT;
+		ephemer_key_parts->is_nullable = false;
+		ephemer_key_parts->sort_order = SORT_ORDER_ASC;
+		ephemer_key_parts->path = NULL;
+		ephemer_key_parts->coll_id = COLL_NONE;
+		ephemer_key_parts->type = FIELD_TYPE_UNSIGNED;
 	}
 	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
-						      field_count, false);
+						      index_size, false);
 	if (ephemer_key_def == NULL)
 		return NULL;
 
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 43a0de5..9dbbb28 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -455,8 +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);
+			sqlVdbeAddOp3(v, OP_OpenTEphemeral, reg_eph,
+				      nColumn + 1, nColumn);
 			addrL = sqlVdbeAddOp1(v, OP_Yield, dest.iSDParm);
 			VdbeCoverage(v);
 			sqlVdbeAddOp2(v, OP_NextIdEphemeral, reg_eph,
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 1ded6c7..333dd1d 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -67,11 +67,13 @@ int tarantoolsqlRenameTrigger(const char *zTriggerName,
  *
  * @param field_count Number of fields in ephemeral space.
  * @param key_info Keys description for new ephemeral space.
+ * @param rowid Rowid column number or 0.
  *
  * @retval Pointer to created space, NULL if error.
  */
 struct space *
-sql_ephemeral_space_create(uint32_t filed_count, struct sql_key_info *key_info);
+sql_ephemeral_space_create(uint32_t filed_count, struct sql_key_info *key_info,
+			   uint32_t rowid);
 
 /**
  * Insert tuple into ephemeral space.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index eab74db..4a1dc74 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2704,11 +2704,20 @@ case OP_Column: {
 		 * Ephemeral spaces feature only one index
 		 * covering all fields, but ephemeral spaces
 		 * lack format. So, we can fetch type from
-		 * key parts.
+		 * key parts. In case of erphemeral space was
+		 * used during INSERT the index of ephemeral space
+		 * consist of only one field - rowid. Its ID is
+		 * not 0 since it added at the end of the tuple.
+		 * Also, in this case we are sure that
+		 * field_type is SCALAR.
 		 */
 		if (pC->uc.pCursor->curFlags & BTCF_TEphemCursor) {
-			field_type = pC->uc.pCursor->index->def->
-					key_def->parts[p2].type;
+			struct key_def *key_def =
+				pC->uc.pCursor->index->def->key_def;
+			if (key_def->parts[0].fieldno != 0)
+				field_type = FIELD_TYPE_SCALAR;
+			else
+				field_type = key_def->parts[p2].type;
 		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
 			field_type = pC->uc.pCursor->space->def->
 					fields[p2].type;
@@ -3148,10 +3157,13 @@ open_cursor_set_hints:
  * Synopsis:
  * @param P1 register, where pointer to new space is stored.
  * @param P2 number of columns in a new table.
+ * @param P3 rowid column number or 0.
  * @param P4 key def for new table, NULL is allowed.
  *
  * This opcode creates Tarantool's ephemeral table and stores pointer
- * to it into P1 register.
+ * to it into P1 register. If P3 is not 0, than rowid is used as
+ * primary index of the ephemeral space. In the other case PK of
+ * the ephemeral space consist of all columns.
  */
 case OP_OpenTEphemeral: {
 	assert(pOp->p1 >= 0);
@@ -3159,7 +3171,8 @@ case OP_OpenTEphemeral: {
 	assert(pOp->p4type != P4_KEYINFO || pOp->p4.key_info != NULL);
 
 	struct space *space = sql_ephemeral_space_create(pOp->p2,
-							 pOp->p4.key_info);
+							 pOp->p4.key_info,
+							 pOp->p3);
 
 	if (space == NULL)
 		goto abort_due_to_error;
diff --git a/test/sql/autoincrement.result b/test/sql/autoincrement.result
index ca3d6f3..7d3e902 100644
--- a/test/sql/autoincrement.result
+++ b/test/sql/autoincrement.result
@@ -78,3 +78,43 @@ seqs = box.space._sequence:select{}
 box.schema.user.drop('user1')
  | ---
  | ...
+
+--
+-- gh-4256: 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);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('CREATE TRIGGER r AFTER INSERT ON t FOR EACH ROW BEGIN SELECT 1; END')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('INSERT INTO t VALUES (1), (NULL), (10), (NULL), (NULL), (3), (NULL);')
+ | ---
+ | - autoincrement_ids:
+ |   - 2
+ |   - 11
+ |   - 12
+ |   - 13
+ |   row_count: 7
+ | ...
+box.execute('SELECT * FROM t;')
+ | ---
+ | - metadata:
+ |   - name: I
+ |     type: integer
+ |   rows:
+ |   - [1]
+ |   - [2]
+ |   - [3]
+ |   - [10]
+ |   - [11]
+ |   - [12]
+ |   - [13]
+ | ...
+box.execute('DROP TABLE t;')
+ | ---
+ | - row_count: 1
+ | ...
diff --git a/test/sql/autoincrement.test.lua b/test/sql/autoincrement.test.lua
index 63a902a..99f4813 100644
--- a/test/sql/autoincrement.test.lua
+++ b/test/sql/autoincrement.test.lua
@@ -31,3 +31,13 @@ seqs = box.space._sequence:select{}
 #seqs == 0 or seqs
 
 box.schema.user.drop('user1')
+
+--
+-- gh-4256: 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);')
+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