[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:17:30 MSK 2020
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?
>
> >
> > 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