> On 28 Mar 2019, at 18:11, Vladislav Shpilevoy <v.shpilevoy@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.