[tarantool-patches] Re: [PATCH v2 4/4] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY
n.pettik
korablev at tarantool.org
Fri Mar 29 21:16:31 MSK 2019
> On 28 Mar 2019, at 18:11, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
>
> Thanks for the patch! See 5 comments below, review fixes at
> the bottom of the email and on the branch.
>
> On 28/03/2019 15:07, Nikita Pettik wrote:
>> Table (aka space) can be created without indexes at least from Lua-land
>> (note that according ANSI SQL table may lack PK). Since there were no
>> facilities to create primary key constraint on already existing table,
>> lets extend ADD CONSTRAINT statement with UNIQUE and PRIMARY KEY
>> clauses. In this case, UNIQUE works exactly in the same way as CREATE
>> UNIQUE INDEX ... statement does. In Tarantool primary index is an index
>> with id == 0, so during execution of ADD CONSTRAINT PRIMARY KEY we check
>> that there is no any entries in _index space and create that one.
>> Otherwise, error is raised.
>>
>> Part of #3097
>
> 1. Why not 'Closes #3097’?
Because ADD CONSTRAINT CHECK is still not yet implemented.
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 20cc346a0..1f604cfe0 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -1956,6 +1956,20 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)
>> return iid_reg;
>> }
>>
>> +static void
>
> 2. A comment is usually required when a function is not a
> one-liner.
>
>> +vdbe_emit_pk_existence_check(struct Parse *parse, uint32_t space_id,
>> + int _index_cursor)
>> +{
>> + struct Vdbe *v = sqlGetVdbe(parse);
>> + int tmp_reg = ++parse->nMem;
>
> 3. Why 'tmp'? As I see, you use a normal register, not temporary.
>
>> + sqlVdbeAddOp2(v, OP_Integer, space_id, tmp_reg);
>> + int found_addr = sqlVdbeAddOp4Int(v, OP_NotFound, _index_cursor, 0,
>> + tmp_reg, 1);
>> + sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
>> + "multiple primary keys are not allowed", P4_STATIC);
>> + sqlVdbeJumpHere(v, found_addr);
>> +}
>> +
>
> 4. I noticed that this function in a nutshell does almost the
> same as vdbe_emit_new_index_id, but treats the result differently.
> It searches for a record in _index. So I extracted that part into
> a separate function vdbe_emit_space_index_search - it positions
> a cursor onto the biggest value in _index with a needed space_id
> and returns two addresses: first to jump from when a record is
> found, and a second to jump from when it is not. IMO it would be
> better in encapsulate _index key creation and lookup since it
> is used in two places.
>
>> /**
>> * Add new index to table's indexes list.
>> * We follow convention that PK comes first in list.
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index 0c4603e83..ed5c05436 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -1624,6 +1624,16 @@ cmd ::= alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP REFERENCES
>> sql_create_foreign_key(pParse);
>> }
>>
>> +cmd ::= alter_add_constraint(N) unique_spec(U) LP sortlist(X) RP. {
>> + create_index_def_init(&pParse->create_index_def, N.table_name, &N.name, X, U,
>> + SORT_ORDER_ASC, false);
>> + sql_create_index(pParse);
>> +}
>> +
>> +%type unique_spec {int}
>
> 5. We have a enum for these values.
Ok. I see you’ve already fixed all points, so I’m
just applying your patch.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20190329/b33b048a/attachment.html>
More information about the Tarantool-patches
mailing list