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

Mergen Imeev imeevma at tarantool.org
Tue Feb 25 13:22:24 MSK 2020


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


More information about the Tarantool-patches mailing list