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’?

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.