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

[Nikita Pettik <korablev@tarantool.org>]

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