From: Mergen Imeev <imeevma@tarantool.org>
To: Nikita Pettik <korablev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: use rowid as PK of ephemeral space on INSERT
Date: Tue, 25 Feb 2020 13:22:24 +0300 [thread overview]
Message-ID: <20200225102224.GB28823@tarantool.org> (raw)
In-Reply-To: <20200225101729.GA28823@tarantool.org>
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@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@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;')
next prev parent reply other threads:[~2020-02-25 10:22 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-17 7:59 imeevma
2020-01-20 17:57 ` Nikita Pettik
2020-02-03 14:00 ` Mergen Imeev
2020-02-25 10:17 ` Mergen Imeev
2020-02-25 10:22 ` Mergen Imeev [this message]
2020-03-11 15:27 ` Nikita Pettik
2020-03-23 11:40 ` Mergen Imeev
2020-03-23 11:42 ` Imeev Mergen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200225102224.GB28823@tarantool.org \
--to=imeevma@tarantool.org \
--cc=korablev@tarantool.org \
--cc=tarantool-patches@dev.tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH v1 1/1] sql: use rowid as PK of ephemeral space on INSERT' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox