[tarantool-patches] Re: [PATCH v2 4/4] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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

> Follow-up #3914
> 
> @TarantoolBot document
> Title: ALTER TABLE ADD CONSTRAINT UNIQUE/PK
> 
> Currently, tables which lack primary key can take part neither in DDL
> nor DQL routines. Attempt to do this leads to error. Such tables
> (without PK) may appear as spaces created from Lua NoSQL interface.
> To improve NoSQL-SQL interoperability, we are introducing way to add
> primary key to already existing table:
> 
> ALTER TABLE <table name> ADD CONSTRAINT <constraint name> PRIMARY KEY(<column list>)
> 
> And alongside with this another one alias to CREATE UNIQUE INDEX:
> 
> ALTER TABLE <table name> ADD CONSTRAINT <constraint name> UNIQUE(<column list>)
> 
> Note that Tarantool PK constraint should be explicitly added before
> any other unique constraints or secondary indexes. Otherwise, error is
> raised: "can not add a secondary key before primary".
> ---
>  src/box/sql/build.c          | 32 +++++++++++++++++++++++--
>  src/box/sql/parse.y          | 10 ++++++++
>  test/sql-tap/alter.test.lua  | 57 +++++++++++++++++++++++++++++++++++++++++++-
>  test/sql-tap/index1.test.lua | 11 ++++++++-
>  4 files changed, 106 insertions(+), 4 deletions(-)
> 
> 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.

> +unique_spec(U) ::= UNIQUE.      { U = SQL_INDEX_TYPE_CONSTRAINT_UNIQUE; }
> +unique_spec(U) ::= PRIMARY KEY. { U = SQL_INDEX_TYPE_CONSTRAINT_PK; }
> +

================================================================

commit cbdb0e2272df7893842a0279ab09d25cdfbddc1e
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Mar 28 17:38:34 2019 +0300

    Review fixes

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 94a559bec..18e438c09 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1918,6 +1918,38 @@ sql_drop_foreign_key(struct Parse *parse_context)
 	sqlVdbeChangeP5(sqlGetVdbe(parse_context), OPFLAG_NCHANGE);
 }
 
+/**
+ * Position @a _index_cursor onto a last record in _index space
+ * with a specified @a space_id. It corresponds to the latest
+ * created index with the biggest id.
+ * @param parser SQL parser.
+ * @param space_id Space identifier to use as a key for _index.
+ * @param _index_cursor A cursor, opened on _index system space.
+ * @param[out] not_found_addr A VDBE address from which a jump
+ *       happens when a record was not found.
+ *
+ * @return A VDBE address from which a jump happens when a record
+ *         was found.
+ */
+static int
+vdbe_emit_space_index_search(struct Parse *parser, uint32_t space_id,
+			     int _index_cursor, int *not_found_addr)
+{
+	struct Vdbe *v = sqlGetVdbe(parser);
+	int key_reg = ++parser->nMem;
+
+	sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
+	int not_found1 = sqlVdbeAddOp4Int(v, OP_SeekLE, _index_cursor, 0,
+					      key_reg, 1);
+	int not_found2 = sqlVdbeAddOp4Int(v, OP_IdxLT, _index_cursor, 0,
+					  key_reg, 1);
+	int found_addr = sqlVdbeAddOp0(v, OP_Goto);
+	sqlVdbeJumpHere(v, not_found1);
+	sqlVdbeJumpHere(v, not_found2);
+	*not_found_addr = sqlVdbeAddOp0(v, OP_Goto);
+	return found_addr;
+}
+
 /**
  * Generate code to determine next free secondary index id in the
  * space identified by @a space_id. Overall VDBE program logic is
@@ -1935,26 +1967,20 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id,
 			   int _index_cursor)
 {
 	struct Vdbe *v = sqlGetVdbe(parse);
-	int key_reg = ++parse->nMem;
-
-	sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
-	int seek_adr = sqlVdbeAddOp4Int(v, OP_SeekLE, _index_cursor, 0,
-					key_reg, 1);
-	sqlVdbeAddOp4Int(v, OP_IdxLT, _index_cursor, 0, key_reg, 1);
-	/* Jump over Halt block. */
-	int goto_succ_addr = sqlVdbeAddOp0(v, OP_Goto);
+	int not_found_addr, found_addr =
+		vdbe_emit_space_index_search(parse, space_id, _index_cursor,
+					     &not_found_addr);
 	/*
 	 * Absence of any records in _index for that space is
 	 * handled here.
 	 */
-	sqlVdbeJumpHere(v, seek_adr);
-	sqlVdbeJumpHere(v, seek_adr + 1);
+	sqlVdbeJumpHere(v, not_found_addr);
 	sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, ON_CONFLICT_ACTION_FAIL,
 		      0, "can not add a secondary key before primary",
 		      P4_STATIC);
 	sqlVdbeChangeP5(v, ER_ALTER_SPACE);
 
-	sqlVdbeJumpHere(v, goto_succ_addr);
+	sqlVdbeJumpHere(v, found_addr);
 	/* Fetch iid from the row and increment it. */
 	int iid_reg = ++parse->nMem;
 	sqlVdbeAddOp3(v, OP_Column, _index_cursor, BOX_INDEX_FIELD_ID, iid_reg);
@@ -1962,18 +1988,28 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id,
 	return iid_reg;
 }
 
-static void
-vdbe_emit_pk_existence_check(struct Parse *parse, uint32_t space_id,
-			     int _index_cursor)
+/**
+ * Generate code that pushes a primary index id 0, but checks if a
+ * space with @a space_id has any indexes. If it does, then the
+ * execution is stopped with an error.
+ *
+ * @return Register holding a new index id.
+ */
+static int
+vdbe_emit_new_pk_index_id(struct Parse *parse, uint32_t space_id,
+			  int _index_cursor)
 {
 	struct Vdbe *v = sqlGetVdbe(parse);
-	int tmp_reg = ++parse->nMem;
-	sqlVdbeAddOp2(v, OP_Integer, space_id, tmp_reg);
-	int found_addr = sqlVdbeAddOp4Int(v, OP_NotFound, _index_cursor, 0,
-					  tmp_reg, 1);
+	int not_found_addr, found_addr =
+		vdbe_emit_space_index_search(parse, space_id, _index_cursor,
+					     &not_found_addr);
+	sqlVdbeJumpHere(v, found_addr);
 	sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
 		      "multiple primary keys are not allowed", P4_STATIC);
-	sqlVdbeJumpHere(v, found_addr);
+	sqlVdbeJumpHere(v, not_found_addr);
+	int index_id = parse->nMem++;
+	sqlVdbeAddOp2(v, OP_Integer, 0, index_id);
+	return index_id;
 }
 
 /**
@@ -2441,9 +2477,8 @@ sql_create_index(struct Parse *parse) {
 		 * we can immediately halt execution of VDBE.
 		 */
 		if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK) {
-			vdbe_emit_pk_existence_check(parse, def->id, cursor);
-			index_id = parse->nMem++;
-			sqlVdbeAddOp2(vdbe, OP_Integer, 0, index_id);
+			index_id = vdbe_emit_new_pk_index_id(parse, def->id,
+							     cursor);
 		} else {
 			index_id = vdbe_emit_new_sec_index_id(parse, def->id,
 							      cursor);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ed5c05436..53718a158 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1630,7 +1630,7 @@ cmd ::= alter_add_constraint(N) unique_spec(U) LP sortlist(X) RP. {
   sql_create_index(pParse);
 }
 
-%type unique_spec {int}
+%type unique_spec {enum sql_index_type}
 unique_spec(U) ::= UNIQUE.      { U = SQL_INDEX_TYPE_CONSTRAINT_UNIQUE; }
 unique_spec(U) ::= PRIMARY KEY. { U = SQL_INDEX_TYPE_CONSTRAINT_PK; }