Re: [Tarantool-patches] [PATCH v2 2/2] sql: do not change order of inserted values

[Nikita Pettik <korablev@tarantool.org>]

On 12 Apr 19:29, imeevma@tarantool.org wrote:
> @@ -365,18 +372,21 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
>  		}
>  	}
>  
> -	for (uint32_t i = 0; i < field_count; ++i) {
> +	for (uint32_t i = 0; i < part_count; ++i) {
>  		struct key_part_def *part = &ephemer_key_parts[i];
> -		part->fieldno = i;

I'd place comment explaining value of j and position of 'rowid'.

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

So why don't you use this trick only if space with autoincrement
is affected? So that proceed with old way for all other spaces.

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

Btw, why don't you use SQL format of tests?

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