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 3B00546970E for ; Mon, 20 Jan 2020 20:57:27 +0300 (MSK) Date: Mon, 20 Jan 2020 20:57:25 +0300 From: Nikita Pettik Message-ID: <20200120175725.GE82780@tarantool.org> References: <23e20a4665756721778df4e46d8efb3d273fde02.1579247383.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <23e20a4665756721778df4e46d8efb3d273fde02.1579247383.git.imeevma@gmail.com> 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: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org 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? > > 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. > + 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. > } > 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? > 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. > 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? > + * 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. > } 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. > * @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. > In the other case PK of > + * the ephemeral space consist of all columns. > */ > case OP_OpenTEphemeral: { > assert(pOp->p1 >= 0);