From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9AAC7469719 for ; Tue, 25 Feb 2020 13:22:29 +0300 (MSK) Date: Tue, 25 Feb 2020 13:22:24 +0300 From: Mergen Imeev Message-ID: <20200225102224.GB28823@tarantool.org> References: <23e20a4665756721778df4e46d8efb3d273fde02.1579247383.git.imeevma@gmail.com> <20200120175725.GE82780@tarantool.org> <20200225101729.GA28823@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200225101729.GA28823@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: use rowid as PK of ephemeral space on INSERT List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.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 > 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;')