[Tarantool-patches] [PATCH v1 1/1] sql: use rowid as PK of ephemeral space on INSERT
Nikita Pettik
korablev at tarantool.org
Mon Jan 20 20:57:25 MSK 2020
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.
> + 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);
More information about the Tarantool-patches
mailing list