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

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

[imeevma]

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

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

[Nikita Pettik]

On 17 Jan 10:59, imeevma@tarantool.org wrote:
> 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.

It would be nice to see particular example with an explanation.

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

How?

> 
> 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)
>  {

You can fit "rowid" in key_info with ease, it would allow to remove
absolutely unnecessary branching. Please elaborate on that.


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

key_part_def_default + set field_type/fieldno ? I doubt that nullable_action
ABORT makes any sense here.

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

Why exactly in this particular case PK is required to be rowid?

>  			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.

Please, add decent comments. "Rowid column number" says nothing.

> 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

Nit: ephemeral.

> +		 * used during INSERT the index of ephemeral space
> +		 * consist of only one field - rowid. It's ID is 

Nit: consists. What is ID?

> +		 * not 0 since it added at the end of the tuple.

Nit: It is added.

Can't parse these two sentences at all. Could you be more specific
and re-phrase them?

> +		 * 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;

This code looks quite obscure.

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

-> rowid field number. In case ephemeral space doesn't feature
   rowid fields, it is 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.

rowid can't be used as PK. PK can be built over field(s) with given fieldno.

> In the other case PK of
> + * the ephemeral space consist of all columns.
>   */
>  case OP_OpenTEphemeral: {
>  	assert(pOp->p1 >= 0);

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

[Mergen Imeev]

Hi! Thank you for review. My answers below. After we settle
them I'll send a new version.

On Mon, Jan 20, 2020 at 08:57:25PM +0300, Nikita Pettik wrote:
> On 17 Jan 10:59, imeevma@tarantool.org wrote:
> > 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.
> 
> It would be nice to see particular example with an explanation.
> 
One can be seen in test for this issue and issue comments:

tarantool> box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
---
- row_count: 1
...

tarantool> box.execute('CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN SELECT 1; END')
---
- row_count: 1
...

tarantool> box.execute('INSERT INTO t1 VALUES (100), (NULL);')
---
- autoincrement_ids:
  - 1
  row_count: 2
...

Obviously 101 should be returned in 'autoincrement_ids:'.

> > This patch fixes sorting in ephemeral space in case it was used in
> > INSERT.
> 
> How?
> 
By using rowid for index for 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)
> >  {
> 
> You can fit "rowid" in key_info with ease, it would allow to remove
> absolutely unnecessary branching. Please elaborate on that.
> 
> 
There is no key_info when ephemeral space created in
insert.c. However, even if I add key_info, I have to
mention parts of index somewhere, since it will be
different from usual one. The first field of this index
should be rowid. Actually, rowid can be the only field
of the index. I do not think that it is possible to
completely remove branching.

> > +	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;
> 
> key_part_def_default + set field_type/fieldno ? I doubt that nullable_action
> ABORT makes any sense here.
> 
In case something goes wrong with rowid we will see that.
Usually, that shouldn't be the case, since each rowid is
unique.

> >  	}
> >  	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);
> 
> Why exactly in this particular case PK is required to be rowid?
> 
Because:
1) In case of UPDATE and DELETE for non-ephemeral spaces,
the ephemeral space contains PK of the original space.
There is no need for rowid.

In case of UPDATE and DELETE for ephemeral spaces, the
unique column created before creation of the ephemeral
space. Actually, we can use the parameter here.

2) In the other cases order does not matter.

> >  			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.
> 
> Please, add decent comments. "Rowid column number" says nothing.
> 
Ok, will do.

> > 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
> 
> Nit: ephemeral.
> 
> > +		 * used during INSERT the index of ephemeral space
> > +		 * consist of only one field - rowid. It's ID is 
> 
> Nit: consists. What is ID?
> 
> > +		 * not 0 since it added at the end of the tuple.
> 
> Nit: It is added.
> 
> Can't parse these two sentences at all. Could you be more specific
> and re-phrase them?
> 
Ok, will do.

> > +		 * 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;
> 
> This code looks quite obscure.
> 
Ok, I will add a comment. Actually, we can say that
field_type here will be SCALAR, since fieldno of part[0]
should be 0 in the other case. The only case when it is
not 0 is when rowid is used for PK.

> >  		} 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.
> 
> -> rowid field number. In case ephemeral space doesn't feature
>    rowid fields, it is 0.
> 
Thanks.

> >   * @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.
> 
> rowid can't be used as PK. PK can be built over field(s) with given fieldno.
> 
Ok, will fix.

> > In the other case PK of
> > + * the ephemeral space consist of all columns.
> >   */
> >  case OP_OpenTEphemeral: {
> >  	assert(pOp->p1 >= 0);

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

[Mergen Imeev]

Hi! Thank you for review. My answers and new patch beliw.
I didn't include diff since there was a lot of changes. 

On Mon, Jan 20, 2020 at 08:57:25PM +0300, Nikita Pettik wrote:
> On 17 Jan 10:59, imeevma@tarantool.org wrote:
> > 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.
> 
> It would be nice to see particular example with an explanation.
> 
> > This patch fixes sorting in ephemeral space in case it was used in
> > INSERT.
> 
> How?
> 
> > 
> > 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)
> >  {
> 
> You can fit "rowid" in key_info with ease, it would allow to remove
> absolutely unnecessary branching. Please elaborate on that.
> 
> 
Done.

> > +	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;
> 
> key_part_def_default + set field_type/fieldno ? I doubt that nullable_action
> ABORT makes any sense here.
> 
Fixed.

> >  	}
> >  	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);
> 
> Why exactly in this particular case PK is required to be rowid?
> 
Rowid should be the first field of PK.

> >  			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.
> 
> Please, add decent comments. "Rowid column number" says nothing.
> 
This change was removed.

> > 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
> 
> Nit: ephemeral.
> 
Removed.

> > +		 * used during INSERT the index of ephemeral space
> > +		 * consist of only one field - rowid. It's ID is 
> 
> Nit: consists. What is ID?
> 
Field number.

> > +		 * not 0 since it added at the end of the tuple.
> 
> Nit: It is added.
> 
> Can't parse these two sentences at all. Could you be more specific
> and re-phrase them?
> 
Done.

> > +		 * 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;
> 
> This code looks quite obscure.
> 
Changed.

> >  		} 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.
> 
> -> rowid field number. In case ephemeral space doesn't feature
>    rowid fields, it is 0.
> 
Removed.

> >   * @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.
> 
> rowid can't be used as PK. PK can be built over field(s) with given fieldno.
> 
Removed.

> > In the other case PK of
> > + * the ephemeral space consist of all columns.
> >   */
> >  case OP_OpenTEphemeral: {
> >  	assert(pOp->p1 >= 0);

New patch:

From 2040bb06d968918549f04abf69a5d8ca8cfd075a Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 24 Feb 2020 16:44:07 +0300
Subject: [PATCH] sql: do not change order of inserted values on INSERT

Prior to this patch, if ephemeral space was used during INSERT,
the inserted values were sorted by the first column. This can lead
to an error when using the autoincrement feature. To avoid this,
the patch makes it possible to make the rowid of the value
inserted into the ephemeral space be the first part of
ephemeral space index.

Closes #4256

diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df8..47cf235 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -338,7 +338,10 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 		return NULL;
 	}
 	for (uint32_t i = 0; i < field_count; ++i) {
-		struct key_part_def *part = &ephemer_key_parts[i];
+		uint32_t j = i;
+		if (key_info != NULL && key_info->is_rowid_first)
+			j = (j + 1) % field_count;
+		struct key_part_def *part = &ephemer_key_parts[j];
 		part->fieldno = i;
 		part->nullable_action = ON_CONFLICT_ACTION_NONE;
 		part->is_nullable = true;
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 43a0de5..6ee728a 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -455,8 +455,17 @@ 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.
+			 */
+			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_rowid_first = 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/src/box/sql/select.c b/src/box/sql/select.c
index 65e41f2..105a4a3 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_rowid_first = 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_rowid_first = 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 d1fcf47..d9da921 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4111,6 +4111,8 @@ struct sql_key_info {
 	struct key_def *key_def;
 	/** Reference counter. */
 	uint32_t refs;
+	/** Rowid should be the first part of PK if true. */
+	bool is_rowid_first;
 	/** 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 620d74e..db782a7 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2702,8 +2702,25 @@ case OP_Column: {
 		 * key parts.
 		 */
 		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;
+			/*
+			 * There are three options:
+			 * |Rowid|First field|...|Last field|
+			 * Or:
+			 * |First field|...|Last field|
+			 * Or:
+			 * |First field|...|Last field|Rowid|
+			 *
+			 * If ephemeral space has a rowid column,
+			 * it is always the last column. Due to
+			 * this, the field number of the rowid
+			 * column cannot be 0.
+			 */
+			int partno = p2;
+			if (key_def->parts[0].fieldno != 0)
+				partno = (partno + 1) % key_def->part_count;
+			field_type = key_def->parts[partno].type;
 		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
 			field_type = pC->uc.pCursor->space->def->
 					fields[p2].type;
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;')

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

[Mergen Imeev]

Sorry, missed one question in the last letter.

On Tue, Feb 25, 2020 at 01:17:29PM +0300, Mergen Imeev wrote:
> Hi! Thank you for review. My answers and new patch beliw.
> I didn't include diff since there was a lot of changes. 
> 
> On Mon, Jan 20, 2020 at 08:57:25PM +0300, Nikita Pettik wrote:
> > On 17 Jan 10:59, imeevma@tarantool.org wrote:
> > > 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.
> > 
> > It would be nice to see particular example with an explanation.
> > 
> > > This patch fixes sorting in ephemeral space in case it was used in
> > > INSERT.
> > 
> > How?
> > 
By making rowid to be the first part of the index.

> > > 
> > > 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)
> > >  {
> > 
> > You can fit "rowid" in key_info with ease, it would allow to remove
> > absolutely unnecessary branching. Please elaborate on that.
> > 
> > 
> Done.
> 
> > > +	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;
> > 
> > key_part_def_default + set field_type/fieldno ? I doubt that nullable_action
> > ABORT makes any sense here.
> > 
> Fixed.
> 
> > >  	}
> > >  	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);
> > 
> > Why exactly in this particular case PK is required to be rowid?
> > 
> Rowid should be the first field of PK.
> 
> > >  			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.
> > 
> > Please, add decent comments. "Rowid column number" says nothing.
> > 
> This change was removed.
> 
> > > 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
> > 
> > Nit: ephemeral.
> > 
> Removed.
> 
> > > +		 * used during INSERT the index of ephemeral space
> > > +		 * consist of only one field - rowid. It's ID is 
> > 
> > Nit: consists. What is ID?
> > 
> Field number.
> 
> > > +		 * not 0 since it added at the end of the tuple.
> > 
> > Nit: It is added.
> > 
> > Can't parse these two sentences at all. Could you be more specific
> > and re-phrase them?
> > 
> Done.
> 
> > > +		 * 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;
> > 
> > This code looks quite obscure.
> > 
> Changed.
> 
> > >  		} 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.
> > 
> > -> rowid field number. In case ephemeral space doesn't feature
> >    rowid fields, it is 0.
> > 
> Removed.
> 
> > >   * @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.
> > 
> > rowid can't be used as PK. PK can be built over field(s) with given fieldno.
> > 
> Removed.
> 
> > > In the other case PK of
> > > + * the ephemeral space consist of all columns.
> > >   */
> > >  case OP_OpenTEphemeral: {
> > >  	assert(pOp->p1 >= 0);
> 
> New patch:
> 
> From 2040bb06d968918549f04abf69a5d8ca8cfd075a Mon Sep 17 00:00:00 2001
> From: Mergen Imeev <imeevma@gmail.com>
> Date: Mon, 24 Feb 2020 16:44:07 +0300
> Subject: [PATCH] sql: do not change order of inserted values on INSERT
> 
> Prior to this patch, if ephemeral space was used during INSERT,
> the inserted values were sorted by the first column. This can lead
> to an error when using the autoincrement feature. To avoid this,
> the patch makes it possible to make the rowid of the value
> inserted into the ephemeral space be the first part of
> ephemeral space index.
> 
> Closes #4256
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 1256df8..47cf235 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -338,7 +338,10 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
>  		return NULL;
>  	}
>  	for (uint32_t i = 0; i < field_count; ++i) {
> -		struct key_part_def *part = &ephemer_key_parts[i];
> +		uint32_t j = i;
> +		if (key_info != NULL && key_info->is_rowid_first)
> +			j = (j + 1) % field_count;
> +		struct key_part_def *part = &ephemer_key_parts[j];
>  		part->fieldno = i;
>  		part->nullable_action = ON_CONFLICT_ACTION_NONE;
>  		part->is_nullable = true;
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 43a0de5..6ee728a 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -455,8 +455,17 @@ 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.
> +			 */
> +			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_rowid_first = 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/src/box/sql/select.c b/src/box/sql/select.c
> index 65e41f2..105a4a3 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_rowid_first = 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_rowid_first = 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 d1fcf47..d9da921 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4111,6 +4111,8 @@ struct sql_key_info {
>  	struct key_def *key_def;
>  	/** Reference counter. */
>  	uint32_t refs;
> +	/** Rowid should be the first part of PK if true. */
> +	bool is_rowid_first;
>  	/** 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 620d74e..db782a7 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2702,8 +2702,25 @@ case OP_Column: {
>  		 * key parts.
>  		 */
>  		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;
> +			/*
> +			 * There are three options:
> +			 * |Rowid|First field|...|Last field|
> +			 * Or:
> +			 * |First field|...|Last field|
> +			 * Or:
> +			 * |First field|...|Last field|Rowid|
> +			 *
> +			 * If ephemeral space has a rowid column,
> +			 * it is always the last column. Due to
> +			 * this, the field number of the rowid
> +			 * column cannot be 0.
> +			 */
> +			int partno = p2;
> +			if (key_def->parts[0].fieldno != 0)
> +				partno = (partno + 1) % key_def->part_count;
> +			field_type = key_def->parts[partno].type;
>  		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
>  			field_type = pC->uc.pCursor->space->def->
>  					fields[p2].type;
> 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;')

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

[Nikita Pettik]

On 25 Feb 13:17, Mergen Imeev wrote:
> Hi! Thank you for review. My answers and new patch beliw.
> Date: Mon, 24 Feb 2020 16:44:07 +0300
> Subject: [PATCH] sql: do not change order of inserted values on INSERT
> 
> Prior to this patch, if ephemeral space was used during INSERT,

Does fix affect only insert, but not replace? Or both?

> the inserted values were sorted by the first column.

Are inserted values are always sorted during insertion op?

> This can lead
> to an error when using the autoincrement feature.

Why? Could you please provide details? Not personally for me,
but in commit message.

> To avoid this,
> the patch makes it possible to make the rowid of the value
> inserted into the ephemeral space be the first part of
> ephemeral space index.

So, before patch rowid could be only last column of table?
And you achieve it extending sql_key_info with additional field?

All these tip-questions should help you to make commit message
more informative.

> Closes #4256

Please attach a changelog to the patch.

> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 1256df8..47cf235 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -338,7 +338,10 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
>  		return NULL;
>  	}
>  	for (uint32_t i = 0; i < field_count; ++i) {
> -		struct key_part_def *part = &ephemer_key_parts[i];
> +		uint32_t j = i;
> +		if (key_info != NULL && key_info->is_rowid_first)
> +			j = (j + 1) % field_count;

What is going on here??

> +		struct key_part_def *part = &ephemer_key_parts[j];
>  		part->fieldno = i;
>  		part->nullable_action = ON_CONFLICT_ACTION_NONE;
>  		part->is_nullable = true;
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 43a0de5..6ee728a 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -455,8 +455,17 @@ 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.
> +			 */

Please, explain here why rowid should come as first column.
See reference example in sql_table_delete_from():

   233			if (is_view) {
   234				/*
   235				 * At this stage SELECT is already materialized
   236				 * into ephemeral table, which has one additional
   237				 * tail field (except for ones specified in view
   238				 * format) which contains sequential ids. These ids
   239				 * are required since selected values may turn out to
   240				 * be non-unique. For instance:
   241				 * CREATE TABLE t (id INT PRIMARY KEY, a INT);
   242				 * INSERT INTO t VALUES (1, 1), (2, 1), (3, 1);
   243				 * CREATE VIEW v AS SELECT a FROM t;

...

> +			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_rowid_first = 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/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index d1fcf47..d9da921 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4111,6 +4111,8 @@ struct sql_key_info {
>  	struct key_def *key_def;
>  	/** Reference counter. */
>  	uint32_t refs;
> +	/** Rowid should be the first part of PK if true. */
> +	bool is_rowid_first;

Otherwise it is last or not presented at all?

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 620d74e..db782a7 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2702,8 +2702,25 @@ case OP_Column: {
>  		 * key parts.
>  		 */
>  		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;
> +			/*
> +			 * There are three options:
> +			 * |Rowid|First field|...|Last field|
> +			 * Or:
> +			 * |First field|...|Last field|
> +			 * Or:
> +			 * |First field|...|Last field|Rowid|
> +			 *
> +			 * If ephemeral space has a rowid column,

But it contradicts schema above.

> +			 * it is always the last column. Due to
> +			 * this, the field number of the rowid
> +			 * column cannot be 0.
> +			 */
> +			int partno = p2;
> +			if (key_def->parts[0].fieldno != 0)
> +				partno = (partno + 1) % key_def->part_count;

See this for the second time, still don't understand what it is
supposed to mean.

> +			field_type = key_def->parts[partno].type;
>  		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
>  			field_type = pC->uc.pCursor->space->def->
>  					fields[p2].type;
> 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);')

Leave explanation why no-op trigger is required. Otherwise it
looks redundant. Also, Kirill forces new convention saying that
each bug fix should be placed at separate file named gh-xxxx-description.

> +box.execute('SELECT * FROM t;')
> +box.execute('DROP TABLE t;')

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

[Mergen Imeev]

Hi! Thank you for the patch. My answers, diff and new
version below.

On Wed, Mar 11, 2020 at 03:27:59PM +0000, Nikita Pettik wrote:
> On 25 Feb 13:17, Mergen Imeev wrote:
> > Hi! Thank you for review. My answers and new patch beliw.
> > Date: Mon, 24 Feb 2020 16:44:07 +0300
> > Subject: [PATCH] sql: do not change order of inserted values on INSERT
> > 
> > Prior to this patch, if ephemeral space was used during INSERT,
> 
> Does fix affect only insert, but not replace? Or both?
It affects both, but the problem appears only during INSERT
since we cannot use REPLACE to insert NULL in field with
AUTOINCREMENT. Changed the commit-message.

> 
> > the inserted values were sorted by the first column.
> 
> Are inserted values are always sorted during insertion op?
No. It says "if ephemeral space was used", is this not
enough?

> 
> > This can lead
> > to an error when using the autoincrement feature.
> 
> Why? Could you please provide details? Not personally for me,
> but in commit message.
Extended the commit message.

> 
> > To avoid this,
> > the patch makes it possible to make the rowid of the value
> > inserted into the ephemeral space be the first part of
> > ephemeral space index.
> 
> So, before patch rowid could be only last column of table?
Now it is also the last column of the table.

> And you achieve it extending sql_key_info with additional field?
Yes.

> 
> All these tip-questions should help you to make commit message
> more informative.
Thanks.

> 
> > Closes #4256
> 
> Please attach a changelog to the patch.
Done.

> 
> > 
> > diff --git a/src/box/sql.c b/src/box/sql.c
> > index 1256df8..47cf235 100644
> > --- a/src/box/sql.c
> > +++ b/src/box/sql.c
> > @@ -338,7 +338,10 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
> >  		return NULL;
> >  	}
> >  	for (uint32_t i = 0; i < field_count; ++i) {
> > -		struct key_part_def *part = &ephemer_key_parts[i];
> > +		uint32_t j = i;
> > +		if (key_info != NULL && key_info->is_rowid_first)
> > +			j = (j + 1) % field_count;
> 
> What is going on here??
Cyclic shift.

> 
> > +		struct key_part_def *part = &ephemer_key_parts[j];
> >  		part->fieldno = i;
> >  		part->nullable_action = ON_CONFLICT_ACTION_NONE;
> >  		part->is_nullable = true;
> > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> > index 43a0de5..6ee728a 100644
> > --- a/src/box/sql/insert.c
> > +++ b/src/box/sql/insert.c
> > @@ -455,8 +455,17 @@ 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.
> > +			 */
> 
> Please, explain here why rowid should come as first column.
> See reference example in sql_table_delete_from():
> 
>    233			if (is_view) {
>    234				/*
>    235				 * At this stage SELECT is already materialized
>    236				 * into ephemeral table, which has one additional
>    237				 * tail field (except for ones specified in view
>    238				 * format) which contains sequential ids. These ids
>    239				 * are required since selected values may turn out to
>    240				 * be non-unique. For instance:
>    241				 * CREATE TABLE t (id INT PRIMARY KEY, a INT);
>    242				 * INSERT INTO t VALUES (1, 1), (2, 1), (3, 1);
>    243				 * CREATE VIEW v AS SELECT a FROM t;
> 
> ...
> 
Done.

> > +			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_rowid_first = 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/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> > index d1fcf47..d9da921 100644
> > --- a/src/box/sql/sqlInt.h
> > +++ b/src/box/sql/sqlInt.h
> > @@ -4111,6 +4111,8 @@ struct sql_key_info {
> >  	struct key_def *key_def;
> >  	/** Reference counter. */
> >  	uint32_t refs;
> > +	/** Rowid should be the first part of PK if true. */
> > +	bool is_rowid_first;
> 
> Otherwise it is last or not presented at all?
Yes. I changed the comment, but I am not sure if this is
right. The only porpose of this flag is to show that if
the rowid the first column. If this flag is false we may
say that rowid may be not the first column.

> 
> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 620d74e..db782a7 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -2702,8 +2702,25 @@ case OP_Column: {
> >  		 * key parts.
> >  		 */
> >  		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;
> > +			/*
> > +			 * There are three options:
> > +			 * |Rowid|First field|...|Last field|
> > +			 * Or:
> > +			 * |First field|...|Last field|
> > +			 * Or:
> > +			 * |First field|...|Last field|Rowid|
> > +			 *
> > +			 * If ephemeral space has a rowid column,
> 
> But it contradicts schema above.
How?

> 
> > +			 * it is always the last column. Due to
> > +			 * this, the field number of the rowid
> > +			 * column cannot be 0.
> > +			 */
> > +			int partno = p2;
> > +			if (key_def->parts[0].fieldno != 0)
> > +				partno = (partno + 1) % key_def->part_count;
> 
> See this for the second time, still don't understand what it is
> supposed to mean.
Simplified a bit and extended the comment.

> 
> > +			field_type = key_def->parts[partno].type;
> >  		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
> >  			field_type = pC->uc.pCursor->space->def->
> >  					fields[p2].type;
> > 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);')
> 
> Leave explanation why no-op trigger is required. Otherwise it
> looks redundant.
Done.

> Also, Kirill forces new convention saying that
> each bug fix should be placed at separate file named gh-xxxx-description.
Done.

> 

> > +box.execute('SELECT * FROM t;')
> > +box.execute('DROP TABLE t;')


Diff:


diff --git a/src/box/sql.c b/src/box/sql.c
index 47cf235..8c25095 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -339,6 +339,12 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	}
 	for (uint32_t i = 0; i < field_count; ++i) {
 		uint32_t j = i;
+		/*
+		 * In case we know that the last column is rowid,
+		 * and we want to make it the first part of the
+		 * index, we will do a cyclic shift. Thus, we will
+		 * not disrupt the order of other columns.
+		 */
 		if (key_info != NULL && key_info->is_rowid_first)
 			j = (j + 1) % field_count;
 		struct key_part_def *part = &ephemer_key_parts[j];
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 6ee728a..28e3096 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -458,6 +458,12 @@ sqlInsert(Parse * pParse,	/* Parser context */
 			/*
 			 * 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.
 			 */
 			struct sql_key_info *key_info =
 				sql_key_info_new(pParse->db, nColumn + 1);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 9c00a7b..a5f0249 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4114,7 +4114,11 @@ struct sql_key_info {
 	struct key_def *key_def;
 	/** Reference counter. */
 	uint32_t refs;
-	/** Rowid should be the first part of PK if true. */
+	/**
+	 * Rowid should be the first part of PK, if true. If this
+	 * flag is false, rowid may be any part of the index or
+	 * may be absent.
+	 */
 	bool is_rowid_first;
 	/** Number of parts in the key. */
 	uint32_t part_count;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index a200063..e5cfa5e 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2715,11 +2715,21 @@ case OP_Column: {
 			 * If ephemeral space has a rowid column,
 			 * it is always the last column. Due to
 			 * this, the field number of the rowid
-			 * column cannot be 0.
+			 * column cannot be 0. So, if the first
+			 * part of the index have something other
+			 * than 0 as a fieldno, we know that this
+			 * is fieldno of rowid, since in all other
+			 * cases fieldno of the first part is 0.
+			 *
+			 * If rowid is the first part of the
+			 * index, we know that all the other parts
+			 * are columns sorted in the required
+			 * order. So, we should increment partno
+			 * by 1.
 			 */
-			int partno = p2;
+			uint32_t partno = p2;
 			if (key_def->parts[0].fieldno != 0)
-				partno = (partno + 1) % key_def->part_count;
+				++partno;
 			field_type = key_def->parts[partno].type;
 		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
 			field_type = pC->uc.pCursor->space->def->
diff --git a/test/sql/autoincrement.result b/test/sql/autoincrement.result
index 7d3e902..ca3d6f3 100644
--- a/test/sql/autoincrement.result
+++ b/test/sql/autoincrement.result
@@ -78,43 +78,3 @@ 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 99f4813..63a902a 100644
--- a/test/sql/autoincrement.test.lua
+++ b/test/sql/autoincrement.test.lua
@@ -31,13 +31,3 @@ 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;')
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..56e28b5
--- /dev/null
+++ b/test/sql/gh-4256-do-not-change-order-during-insertion.result
@@ -0,0 +1,50 @@
+-- test-run result file version 2
+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);')
+ | ---
+ | - row_count: 1
+ | ...
+--
+-- 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')
+ | ---
+ | - 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/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 @@
+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;')


New version:


From 4244c170e0b22fa1084d8d9e1fac9c252b9b73cd Mon Sep 17 00:00:00 2001
Message-Id: <4244c170e0b22fa1084d8d9e1fac9c252b9b73cd.1584962777.git.imeevma@gmail.com>
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 24 Feb 2020 16:44:07 +0300
Subject: [PATCH v2 1/1] 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 first part of the ephemeral space
index. Currently, if the ephemeral space has a rowid, it is always
the last column of the ephemeral space. So, to make rowid the
first part of the index, a cyclic shift is used. Thus, we will not
change the order of all other columns.

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

@ChangeLog
 - During INSERT and REPLACE the order of inserted rows will not 
   be changed (gh-4256).

 src/box/sql.c                                      | 11 ++++-
 src/box/sql/insert.c                               | 19 +++++++-
 src/box/sql/select.c                               |  2 +
 src/box/sql/sqlInt.h                               |  6 +++
 src/box/sql/vdbe.c                                 | 31 +++++++++++++-
 ...256-do-not-change-order-during-insertion.result | 50 ++++++++++++++++++++++
 ...6-do-not-change-order-during-insertion.test.lua | 15 +++++++
 7 files changed, 129 insertions(+), 5 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.lua

diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df8..8c25095 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -338,7 +338,16 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 		return NULL;
 	}
 	for (uint32_t i = 0; i < field_count; ++i) {
-		struct key_part_def *part = &ephemer_key_parts[i];
+		uint32_t j = i;
+		/*
+		 * In case we know that the last column is rowid,
+		 * and we want to make it the first part of the
+		 * index, we will do a cyclic shift. Thus, we will
+		 * not disrupt the order of other columns.
+		 */
+		if (key_info != NULL && key_info->is_rowid_first)
+			j = (j + 1) % field_count;
+		struct key_part_def *part = &ephemer_key_parts[j];
 		part->fieldno = i;
 		part->nullable_action = ON_CONFLICT_ACTION_NONE;
 		part->is_nullable = true;
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 43a0de5..28e3096 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.
+			 */
+			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_rowid_first = 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/src/box/sql/select.c b/src/box/sql/select.c
index 65e41f2..105a4a3 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_rowid_first = 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_rowid_first = 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 1579cc9..a5f0249 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4114,6 +4114,12 @@ struct sql_key_info {
 	struct key_def *key_def;
 	/** Reference counter. */
 	uint32_t refs;
+	/**
+	 * Rowid should be the first part of PK, if true. If this
+	 * flag is false, rowid may be any part of the index or
+	 * may be absent.
+	 */
+	bool is_rowid_first;
 	/** 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 e8a029a..e5cfa5e 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2702,8 +2702,35 @@ case OP_Column: {
 		 * key parts.
 		 */
 		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;
+			/*
+			 * There are three options:
+			 * |Rowid|First field|...|Last field|
+			 * Or:
+			 * |First field|...|Last field|
+			 * Or:
+			 * |First field|...|Last field|Rowid|
+			 *
+			 * If ephemeral space has a rowid column,
+			 * it is always the last column. Due to
+			 * this, the field number of the rowid
+			 * column cannot be 0. So, if the first
+			 * part of the index have something other
+			 * than 0 as a fieldno, we know that this
+			 * is fieldno of rowid, since in all other
+			 * cases fieldno of the first part is 0.
+			 *
+			 * If rowid is the first part of the
+			 * index, we know that all the other parts
+			 * are columns sorted in the required
+			 * order. So, we should increment partno
+			 * by 1.
+			 */
+			uint32_t partno = p2;
+			if (key_def->parts[0].fieldno != 0)
+				++partno;
+			field_type = key_def->parts[partno].type;
 		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
 			field_type = pC->uc.pCursor->space->def->
 					fields[p2].type;
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..56e28b5
--- /dev/null
+++ b/test/sql/gh-4256-do-not-change-order-during-insertion.result
@@ -0,0 +1,50 @@
+-- test-run result file version 2
+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);')
+ | ---
+ | - row_count: 1
+ | ...
+--
+-- 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')
+ | ---
+ | - 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/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 @@
+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

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

[Imeev Mergen]

On 3/23/20 2:40 PM, Mergen Imeev wrote:
> Hi! Thank you for the patch. My answers, diff and new
> version below.

Sorry, I mean "the review".


>
> On Wed, Mar 11, 2020 at 03:27:59PM +0000, Nikita Pettik wrote:
>> On 25 Feb 13:17, Mergen Imeev wrote:
>>> Hi! Thank you for review. My answers and new patch beliw.
>>> Date: Mon, 24 Feb 2020 16:44:07 +0300
>>> Subject: [PATCH] sql: do not change order of inserted values on INSERT
>>>
>>> Prior to this patch, if ephemeral space was used during INSERT,
>> Does fix affect only insert, but not replace? Or both?
> It affects both, but the problem appears only during INSERT
> since we cannot use REPLACE to insert NULL in field with
> AUTOINCREMENT. Changed the commit-message.
>
>>> the inserted values were sorted by the first column.
>> Are inserted values are always sorted during insertion op?
> No. It says "if ephemeral space was used", is this not
> enough?
>
>>> This can lead
>>> to an error when using the autoincrement feature.
>> Why? Could you please provide details? Not personally for me,
>> but in commit message.
> Extended the commit message.
>
>>> To avoid this,
>>> the patch makes it possible to make the rowid of the value
>>> inserted into the ephemeral space be the first part of
>>> ephemeral space index.
>> So, before patch rowid could be only last column of table?
> Now it is also the last column of the table.
>
>> And you achieve it extending sql_key_info with additional field?
> Yes.
>
>> All these tip-questions should help you to make commit message
>> more informative.
> Thanks.
>
>>> Closes #4256
>> Please attach a changelog to the patch.
> Done.
>
>>> diff --git a/src/box/sql.c b/src/box/sql.c
>>> index 1256df8..47cf235 100644
>>> --- a/src/box/sql.c
>>> +++ b/src/box/sql.c
>>> @@ -338,7 +338,10 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
>>>   		return NULL;
>>>   	}
>>>   	for (uint32_t i = 0; i < field_count; ++i) {
>>> -		struct key_part_def *part = &ephemer_key_parts[i];
>>> +		uint32_t j = i;
>>> +		if (key_info != NULL && key_info->is_rowid_first)
>>> +			j = (j + 1) % field_count;
>> What is going on here??
> Cyclic shift.
>
>>> +		struct key_part_def *part = &ephemer_key_parts[j];
>>>   		part->fieldno = i;
>>>   		part->nullable_action = ON_CONFLICT_ACTION_NONE;
>>>   		part->is_nullable = true;
>>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>>> index 43a0de5..6ee728a 100644
>>> --- a/src/box/sql/insert.c
>>> +++ b/src/box/sql/insert.c
>>> @@ -455,8 +455,17 @@ 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.
>>> +			 */
>> Please, explain here why rowid should come as first column.
>> See reference example in sql_table_delete_from():
>>
>>     233			if (is_view) {
>>     234				/*
>>     235				 * At this stage SELECT is already materialized
>>     236				 * into ephemeral table, which has one additional
>>     237				 * tail field (except for ones specified in view
>>     238				 * format) which contains sequential ids. These ids
>>     239				 * are required since selected values may turn out to
>>     240				 * be non-unique. For instance:
>>     241				 * CREATE TABLE t (id INT PRIMARY KEY, a INT);
>>     242				 * INSERT INTO t VALUES (1, 1), (2, 1), (3, 1);
>>     243				 * CREATE VIEW v AS SELECT a FROM t;
>>
>> ...
>>
> Done.
>
>>> +			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_rowid_first = 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/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>>> index d1fcf47..d9da921 100644
>>> --- a/src/box/sql/sqlInt.h
>>> +++ b/src/box/sql/sqlInt.h
>>> @@ -4111,6 +4111,8 @@ struct sql_key_info {
>>>   	struct key_def *key_def;
>>>   	/** Reference counter. */
>>>   	uint32_t refs;
>>> +	/** Rowid should be the first part of PK if true. */
>>> +	bool is_rowid_first;
>> Otherwise it is last or not presented at all?
> Yes. I changed the comment, but I am not sure if this is
> right. The only porpose of this flag is to show that if
> the rowid the first column. If this flag is false we may
> say that rowid may be not the first column.
>
>>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>>> index 620d74e..db782a7 100644
>>> --- a/src/box/sql/vdbe.c
>>> +++ b/src/box/sql/vdbe.c
>>> @@ -2702,8 +2702,25 @@ case OP_Column: {
>>>   		 * key parts.
>>>   		 */
>>>   		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;
>>> +			/*
>>> +			 * There are three options:
>>> +			 * |Rowid|First field|...|Last field|
>>> +			 * Or:
>>> +			 * |First field|...|Last field|
>>> +			 * Or:
>>> +			 * |First field|...|Last field|Rowid|
>>> +			 *
>>> +			 * If ephemeral space has a rowid column,
>> But it contradicts schema above.
> How?
>
>>> +			 * it is always the last column. Due to
>>> +			 * this, the field number of the rowid
>>> +			 * column cannot be 0.
>>> +			 */
>>> +			int partno = p2;
>>> +			if (key_def->parts[0].fieldno != 0)
>>> +				partno = (partno + 1) % key_def->part_count;
>> See this for the second time, still don't understand what it is
>> supposed to mean.
> Simplified a bit and extended the comment.
>
>>> +			field_type = key_def->parts[partno].type;
>>>   		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
>>>   			field_type = pC->uc.pCursor->space->def->
>>>   					fields[p2].type;
>>> 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);')
>> Leave explanation why no-op trigger is required. Otherwise it
>> looks redundant.
> Done.
>
>> Also, Kirill forces new convention saying that
>> each bug fix should be placed at separate file named gh-xxxx-description.
> Done.
>
>>> +box.execute('SELECT * FROM t;')
>>> +box.execute('DROP TABLE t;')
>
> Diff:
>
>
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 47cf235..8c25095 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -339,6 +339,12 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
>   	}
>   	for (uint32_t i = 0; i < field_count; ++i) {
>   		uint32_t j = i;
> +		/*
> +		 * In case we know that the last column is rowid,
> +		 * and we want to make it the first part of the
> +		 * index, we will do a cyclic shift. Thus, we will
> +		 * not disrupt the order of other columns.
> +		 */
>   		if (key_info != NULL && key_info->is_rowid_first)
>   			j = (j + 1) % field_count;
>   		struct key_part_def *part = &ephemer_key_parts[j];
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 6ee728a..28e3096 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -458,6 +458,12 @@ sqlInsert(Parse * pParse,	/* Parser context */
>   			/*
>   			 * 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.
>   			 */
>   			struct sql_key_info *key_info =
>   				sql_key_info_new(pParse->db, nColumn + 1);
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 9c00a7b..a5f0249 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4114,7 +4114,11 @@ struct sql_key_info {
>   	struct key_def *key_def;
>   	/** Reference counter. */
>   	uint32_t refs;
> -	/** Rowid should be the first part of PK if true. */
> +	/**
> +	 * Rowid should be the first part of PK, if true. If this
> +	 * flag is false, rowid may be any part of the index or
> +	 * may be absent.
> +	 */
>   	bool is_rowid_first;
>   	/** Number of parts in the key. */
>   	uint32_t part_count;
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index a200063..e5cfa5e 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2715,11 +2715,21 @@ case OP_Column: {
>   			 * If ephemeral space has a rowid column,
>   			 * it is always the last column. Due to
>   			 * this, the field number of the rowid
> -			 * column cannot be 0.
> +			 * column cannot be 0. So, if the first
> +			 * part of the index have something other
> +			 * than 0 as a fieldno, we know that this
> +			 * is fieldno of rowid, since in all other
> +			 * cases fieldno of the first part is 0.
> +			 *
> +			 * If rowid is the first part of the
> +			 * index, we know that all the other parts
> +			 * are columns sorted in the required
> +			 * order. So, we should increment partno
> +			 * by 1.
>   			 */
> -			int partno = p2;
> +			uint32_t partno = p2;
>   			if (key_def->parts[0].fieldno != 0)
> -				partno = (partno + 1) % key_def->part_count;
> +				++partno;
>   			field_type = key_def->parts[partno].type;
>   		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
>   			field_type = pC->uc.pCursor->space->def->
> diff --git a/test/sql/autoincrement.result b/test/sql/autoincrement.result
> index 7d3e902..ca3d6f3 100644
> --- a/test/sql/autoincrement.result
> +++ b/test/sql/autoincrement.result
> @@ -78,43 +78,3 @@ 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 99f4813..63a902a 100644
> --- a/test/sql/autoincrement.test.lua
> +++ b/test/sql/autoincrement.test.lua
> @@ -31,13 +31,3 @@ 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;')
> 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..56e28b5
> --- /dev/null
> +++ b/test/sql/gh-4256-do-not-change-order-during-insertion.result
> @@ -0,0 +1,50 @@
> +-- test-run result file version 2
> +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);')
> + | ---
> + | - row_count: 1
> + | ...
> +--
> +-- 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')
> + | ---
> + | - 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/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 @@
> +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;')
>
>
> New version:
>
>
>  From 4244c170e0b22fa1084d8d9e1fac9c252b9b73cd Mon Sep 17 00:00:00 2001
> Message-Id: <4244c170e0b22fa1084d8d9e1fac9c252b9b73cd.1584962777.git.imeevma@gmail.com>
> From: Mergen Imeev <imeevma@gmail.com>
> Date: Mon, 24 Feb 2020 16:44:07 +0300
> Subject: [PATCH v2 1/1] 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 first part of the ephemeral space
> index. Currently, if the ephemeral space has a rowid, it is always
> the last column of the ephemeral space. So, to make rowid the
> first part of the index, a cyclic shift is used. Thus, we will not
> change the order of all other columns.
>
> Closes #4256
> ---
> https://github.com/tarantool/tarantool/issues/4256
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4256-fix-order-during-insertion
>
> @ChangeLog
>   - During INSERT and REPLACE the order of inserted rows will not
>     be changed (gh-4256).
>
>   src/box/sql.c                                      | 11 ++++-
>   src/box/sql/insert.c                               | 19 +++++++-
>   src/box/sql/select.c                               |  2 +
>   src/box/sql/sqlInt.h                               |  6 +++
>   src/box/sql/vdbe.c                                 | 31 +++++++++++++-
>   ...256-do-not-change-order-during-insertion.result | 50 ++++++++++++++++++++++
>   ...6-do-not-change-order-during-insertion.test.lua | 15 +++++++
>   7 files changed, 129 insertions(+), 5 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.lua
>
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 1256df8..8c25095 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -338,7 +338,16 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
>   		return NULL;
>   	}
>   	for (uint32_t i = 0; i < field_count; ++i) {
> -		struct key_part_def *part = &ephemer_key_parts[i];
> +		uint32_t j = i;
> +		/*
> +		 * In case we know that the last column is rowid,
> +		 * and we want to make it the first part of the
> +		 * index, we will do a cyclic shift. Thus, we will
> +		 * not disrupt the order of other columns.
> +		 */
> +		if (key_info != NULL && key_info->is_rowid_first)
> +			j = (j + 1) % field_count;
> +		struct key_part_def *part = &ephemer_key_parts[j];
>   		part->fieldno = i;
>   		part->nullable_action = ON_CONFLICT_ACTION_NONE;
>   		part->is_nullable = true;
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 43a0de5..28e3096 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.
> +			 */
> +			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_rowid_first = 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/src/box/sql/select.c b/src/box/sql/select.c
> index 65e41f2..105a4a3 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_rowid_first = 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_rowid_first = 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 1579cc9..a5f0249 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4114,6 +4114,12 @@ struct sql_key_info {
>   	struct key_def *key_def;
>   	/** Reference counter. */
>   	uint32_t refs;
> +	/**
> +	 * Rowid should be the first part of PK, if true. If this
> +	 * flag is false, rowid may be any part of the index or
> +	 * may be absent.
> +	 */
> +	bool is_rowid_first;
>   	/** 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 e8a029a..e5cfa5e 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2702,8 +2702,35 @@ case OP_Column: {
>   		 * key parts.
>   		 */
>   		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;
> +			/*
> +			 * There are three options:
> +			 * |Rowid|First field|...|Last field|
> +			 * Or:
> +			 * |First field|...|Last field|
> +			 * Or:
> +			 * |First field|...|Last field|Rowid|
> +			 *
> +			 * If ephemeral space has a rowid column,
> +			 * it is always the last column. Due to
> +			 * this, the field number of the rowid
> +			 * column cannot be 0. So, if the first
> +			 * part of the index have something other
> +			 * than 0 as a fieldno, we know that this
> +			 * is fieldno of rowid, since in all other
> +			 * cases fieldno of the first part is 0.
> +			 *
> +			 * If rowid is the first part of the
> +			 * index, we know that all the other parts
> +			 * are columns sorted in the required
> +			 * order. So, we should increment partno
> +			 * by 1.
> +			 */
> +			uint32_t partno = p2;
> +			if (key_def->parts[0].fieldno != 0)
> +				++partno;
> +			field_type = key_def->parts[partno].type;
>   		} else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
>   			field_type = pC->uc.pCursor->space->def->
>   					fields[p2].type;
> 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..56e28b5
> --- /dev/null
> +++ b/test/sql/gh-4256-do-not-change-order-during-insertion.result
> @@ -0,0 +1,50 @@
> +-- test-run result file version 2
> +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);')
> + | ---
> + | - row_count: 1
> + | ...
> +--
> +-- 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')
> + | ---
> + | - 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/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 @@
> +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;')