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

[Mergen Imeev <imeevma@tarantool.org>]

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