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

[Nikita Pettik <korablev@tarantool.org>]

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