[tarantool-patches] [PATCH v2 0/4] Introduce ALTER TABLE ADD CONSTRAINT PK/UNIQUE

[tarantool-patches] [PATCH v2 0/4] Introduce ALTER TABLE ADD CONSTRAINT PK/UNIQUE

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/gh-3914-fix-create-index-v2
Issue: https://github.com/tarantool/tarantool/issues/3914

These patches haven't been significantly changed since previous
version. The main change is rebase on fresh 2.1.1

Nikita Pettik (4):
  sql: rework ALTER TABLE grammar
  sql: refactor getNewIid() function
  sql: fix error message for improperly created index
  sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY

 src/box/sql/build.c          | 99 +++++++++++++++++++++++++++-----------------
 src/box/sql/parse.y          | 41 ++++++++++++++----
 test/sql-tap/alter.test.lua  | 57 ++++++++++++++++++++++++-
 test/sql-tap/alter2.test.lua |  2 +-
 test/sql-tap/index1.test.lua | 28 ++++++++++++-
 5 files changed, 180 insertions(+), 47 deletions(-)

-- 
2.15.1

[tarantool-patches] [PATCH v2 1/4] sql: rework ALTER TABLE grammar

[Nikita Pettik]

Since ALTER TABLE ADD CONSTRAINT is going to be used to add various
constraint types (foreign key, unique etc), let's a bit refactor it to
make it more extendable.

Needed for #3097
---
 src/box/sql/parse.y          | 31 ++++++++++++++++++++++++-------
 test/sql-tap/alter2.test.lua |  2 +-
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 4eff61777..0c4603e83 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1600,18 +1600,35 @@ cmd ::= DROP TRIGGER ifexists(NOERR) fullname(X). {
 }
 
 //////////////////////// ALTER TABLE table ... ////////////////////////////////
-cmd ::= ALTER TABLE fullname(X) RENAME TO nm(Z). {
-  rename_entity_def_init(&pParse->rename_entity_def, X, &Z);
-  sql_alter_table_rename(pParse);
+%include {
+  struct alter_args {
+    struct SrcList *table_name;
+    /** Name of constraint OR new name of table in case of RENAME. */
+    struct Token name;
+  };
+}
+
+%type alter_table_start {struct SrcList *}
+alter_table_start(A) ::= ALTER TABLE fullname(T) . { A = T; }
+
+%type alter_add_constraint {struct alter_args}
+alter_add_constraint(A) ::= alter_table_start(T) ADD CONSTRAINT nm(N). {
+  A.table_name = T;
+  A.name = N;
 }
 
-cmd ::= ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY
-        LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) matcharg(M)
-        refargs(R) defer_subclause_opt(D). {
-  create_fk_def_init(&pParse->create_fk_def, X, &Z, FA, &T, TA, M, R, D);
+cmd ::= alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP REFERENCES
+        nm(T) eidlist_opt(TA) matcharg(M) refargs(R) defer_subclause_opt(D). {
+  create_fk_def_init(&pParse->create_fk_def, N.table_name, &N.name, FA, &T, TA,
+                     M, R, D);
   sql_create_foreign_key(pParse);
 }
 
+cmd ::= alter_table_start(A) RENAME TO nm(N). {
+    rename_entity_def_init(&pParse->rename_entity_def, A, &N);
+    sql_alter_table_rename(pParse);
+}
+
 cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
   drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
   sql_drop_foreign_key(pParse);
diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
index beb6f5d8a..cc1ff5088 100755
--- a/test/sql-tap/alter2.test.lua
+++ b/test/sql-tap/alter2.test.lua
@@ -223,7 +223,7 @@ test:do_catchsql_test(
         ALTER TABLE child ADD CONSTRAINT fk FOREIGN KEY REFERENCES child(id);
     ]], {
         -- <alter2-4.1>
-        1, "Syntax error near 'REFERENCES'"
+        1, "Keyword 'REFERENCES' is reserved. Please use double quotes if 'REFERENCES' is an identifier."
         -- </alter2-4.1>
     })
 
-- 
2.15.1

[tarantool-patches] [PATCH v2 2/4] sql: refactor getNewIid() function

[Nikita Pettik]

This commit includes no functional changes. Lets simply rewrite
getNewIid() function according to Tarantool codestyle.

Part of #3914
---
 src/box/sql/build.c | 72 ++++++++++++++++++++++++++---------------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 8fb001433..f55f6d800 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1918,45 +1918,43 @@ sql_drop_foreign_key(struct Parse *parse_context)
 	sqlVdbeChangeP5(sqlGetVdbe(parse_context), OPFLAG_NCHANGE);
 }
 
-/*
- * Generate code to determine next free Iid in the space identified by
- * the iSpaceId. Return register number holding the result.
+/**
+ * Generate code to determine next free index id in
+ * the space identified by the @space_id.
+ * Return register holding the result.
+ *
+ * Overall VDBE program logic is following:
+ *
+ * 1 Seek for space id in _index, goto l1 if seeks fails.
+ * 2 Goto l2.
+ * 3 l1: Halt.
+ * 4 l2: Fetch index id from _index record.
  */
 static int
-getNewIid(Parse * pParse, int iSpaceId, int iCursor)
+generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)
 {
-	Vdbe *v = sqlGetVdbe(pParse);
-	int iRes = ++pParse->nMem;
-	int iKey = ++pParse->nMem;
-	int iSeekInst, iGotoInst;
-
-	sqlVdbeAddOp2(v, OP_Integer, iSpaceId, iKey);
-	iSeekInst = sqlVdbeAddOp4Int(v, OP_SeekLE, iCursor, 0, iKey, 1);
-	sqlVdbeAddOp4Int(v, OP_IdxLT, iCursor, 0, iKey, 1);
-
-	/*
-	 * If SeekLE succeeds, the control falls through here, skipping
-	 * IdxLt.
-	 *
-	 * If it fails (no entry with the given key prefix: invalid spaceId)
-	 * VDBE jumps to the next code block (jump target is IMM, fixed up
-	 * later with sqlVdbeJumpHere()).
-	 */
-	iGotoInst = sqlVdbeAddOp0(v, OP_Goto);	/* Jump over Halt */
-
-	/* Invalid spaceId detected. Halt now. */
-	sqlVdbeJumpHere(v, iSeekInst);
-	sqlVdbeJumpHere(v, iSeekInst + 1);
-	sqlVdbeAddOp4(v,
-			  OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
-			  sqlMPrintf(pParse->db, "Invalid space id: %d",
-					 iSpaceId), P4_DYNAMIC);
-
-	/* Fetch iid from the row and ++it. */
-	sqlVdbeJumpHere(v, iGotoInst);
-	sqlVdbeAddOp3(v, OP_Column, iCursor, 1, iRes);
-	sqlVdbeAddOp2(v, OP_AddImm, iRes, 1);
-	return iRes;
+	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, cursor, 0,
+					    key_reg, 1);
+	sqlVdbeAddOp4Int(v, OP_IdxLT, cursor, 0, key_reg, 1);
+	/* Jump over Halt block. */
+	int goto_succ_addr = sqlVdbeAddOp0(v, OP_Goto);
+	/* Invalid space id handling block starts here. */
+	sqlVdbeJumpHere(v, seek_adr);
+	sqlVdbeJumpHere(v, seek_adr + 1);
+	sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
+		      sqlMPrintf(parse->db, "Invalid space id: %d", space_id),
+		      P4_DYNAMIC);
+
+	sqlVdbeJumpHere(v, goto_succ_addr);
+	/* Fetch iid from the row and increment it. */
+	int iid_reg = ++parse->nMem;
+	sqlVdbeAddOp3(v, OP_Column, cursor, BOX_INDEX_FIELD_ID, iid_reg);
+	sqlVdbeAddOp2(v, OP_AddImm, iid_reg, 1);
+	return iid_reg;
 }
 
 /**
@@ -2416,7 +2414,7 @@ sql_create_index(struct Parse *parse) {
 				  (void *)space_by_id(BOX_INDEX_ID),
 				  P4_SPACEPTR);
 		sqlVdbeChangeP5(vdbe, OPFLAG_SEEKEQ);
-		int index_id = getNewIid(parse, def->id, cursor);
+		int index_id = generate_index_id(parse, def->id, cursor);
 		sqlVdbeAddOp1(vdbe, OP_Close, cursor);
 		vdbe_emit_create_index(parse, def, index->def, def->id, index_id);
 		sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE);
-- 
2.15.1

[tarantool-patches] Re: [PATCH v2 2/4] sql: refactor getNewIid() function

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 4 comments below, review fixes
at the end of the email, and on the branch.

On 28/03/2019 15:07, Nikita Pettik wrote:
> This commit includes no functional changes. Lets simply rewrite
> getNewIid() function according to Tarantool codestyle.
> 
> Part of #3914
> ---
>  src/box/sql/build.c | 72 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 35 insertions(+), 37 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 8fb001433..f55f6d800 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1918,45 +1918,43 @@ sql_drop_foreign_key(struct Parse *parse_context)
>  	sqlVdbeChangeP5(sqlGetVdbe(parse_context), OPFLAG_NCHANGE);
>  }
>  
> -/*
> - * Generate code to determine next free Iid in the space identified by
> - * the iSpaceId. Return register number holding the result.
> +/**
> + * Generate code to determine next free index id in
> + * the space identified by the @space_id.

1. According to doxygen docs http://www.doxygen.nl/manual/commands.html
there is no such a command as @space_id. But obviously I know what
you've implied here. For references to parameters we use '@a' before a
parameter. Like this:

    ...
    * the space identifier by @a space_id.
    ...

> + * Return register holding the result.
> + *
> + * Overall VDBE program logic is following:
> + *
> + * 1 Seek for space id in _index, goto l1 if seeks fails.
> + * 2 Goto l2.
> + * 3 l1: Halt.
> + * 4 l2: Fetch index id from _index record.
>   */
>  static int
> -getNewIid(Parse * pParse, int iSpaceId, int iCursor)
> +generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)

2. For vdbe methods generating code we use prefix 'vdbe_emit_'. And you
use it in the last patch btw.

3. In fact, that function generates an index_id for only secondary
indexes. Taking into account these two comments I renamed it to
vdbe_emit_new_sec_index_id().

>  {
> -	Vdbe *v = sqlGetVdbe(pParse);
> -	int iRes = ++pParse->nMem;
> -	int iKey = ++pParse->nMem;
> -	int iSeekInst, iGotoInst;
> -
> -	sqlVdbeAddOp2(v, OP_Integer, iSpaceId, iKey);
> -	iSeekInst = sqlVdbeAddOp4Int(v, OP_SeekLE, iCursor, 0, iKey, 1);
> -	sqlVdbeAddOp4Int(v, OP_IdxLT, iCursor, 0, iKey, 1);
> -
> -	/*
> -	 * If SeekLE succeeds, the control falls through here, skipping
> -	 * IdxLt.
> -	 *
> -	 * If it fails (no entry with the given key prefix: invalid spaceId)
> -	 * VDBE jumps to the next code block (jump target is IMM, fixed up
> -	 * later with sqlVdbeJumpHere()).
> -	 */
> -	iGotoInst = sqlVdbeAddOp0(v, OP_Goto);	/* Jump over Halt */
> -
> -	/* Invalid spaceId detected. Halt now. */
> -	sqlVdbeJumpHere(v, iSeekInst);
> -	sqlVdbeJumpHere(v, iSeekInst + 1);
> -	sqlVdbeAddOp4(v,
> -			  OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
> -			  sqlMPrintf(pParse->db, "Invalid space id: %d",
> -					 iSpaceId), P4_DYNAMIC);
> -
> -	/* Fetch iid from the row and ++it. */
> -	sqlVdbeJumpHere(v, iGotoInst);
> -	sqlVdbeAddOp3(v, OP_Column, iCursor, 1, iRes);
> -	sqlVdbeAddOp2(v, OP_AddImm, iRes, 1);
> -	return iRes;
> +	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, cursor, 0,
> +					    key_reg, 1);

4. Indentation.

> +	sqlVdbeAddOp4Int(v, OP_IdxLT, cursor, 0, key_reg, 1);
> +	/* Jump over Halt block. */
> +	int goto_succ_addr = sqlVdbeAddOp0(v, OP_Goto);
> +	/* Invalid space id handling block starts here. */
> +	sqlVdbeJumpHere(v, seek_adr);
> +	sqlVdbeJumpHere(v, seek_adr + 1);
> +	sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
> +		      sqlMPrintf(parse->db, "Invalid space id: %d", space_id),
> +		      P4_DYNAMIC);
> +

Note that I deliberately used @return instead of @retval, because
@return has no arguments - it is just a description.

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

commit 9e77985d09e53332d87b323c6ef74b747c418d8e
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Mar 28 15:53:24 2019 +0300

    Review fix

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index f55f6d800..b86fcb68d 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1919,27 +1919,28 @@ sql_drop_foreign_key(struct Parse *parse_context)
 }
 
 /**
- * Generate code to determine next free index id in
- * the space identified by the @space_id.
- * Return register holding the result.
- *
- * Overall VDBE program logic is following:
+ * Generate code to determine next free secondary index id in the
+ * space identified by @a space_id. Overall VDBE program logic is
+ * following:
  *
  * 1 Seek for space id in _index, goto l1 if seeks fails.
  * 2 Goto l2.
  * 3 l1: Halt.
  * 4 l2: Fetch index id from _index record.
+ *
+ * @return Register holding a new index id.
  */
 static int
-generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)
+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, cursor, 0,
-					    key_reg, 1);
-	sqlVdbeAddOp4Int(v, OP_IdxLT, cursor, 0, key_reg, 1);
+	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);
 	/* Invalid space id handling block starts here. */
@@ -1952,7 +1953,7 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)
 	sqlVdbeJumpHere(v, goto_succ_addr);
 	/* Fetch iid from the row and increment it. */
 	int iid_reg = ++parse->nMem;
-	sqlVdbeAddOp3(v, OP_Column, cursor, BOX_INDEX_FIELD_ID, iid_reg);
+	sqlVdbeAddOp3(v, OP_Column, _index_cursor, BOX_INDEX_FIELD_ID, iid_reg);
 	sqlVdbeAddOp2(v, OP_AddImm, iid_reg, 1);
 	return iid_reg;
 }
@@ -2414,7 +2415,8 @@ sql_create_index(struct Parse *parse) {
 				  (void *)space_by_id(BOX_INDEX_ID),
 				  P4_SPACEPTR);
 		sqlVdbeChangeP5(vdbe, OPFLAG_SEEKEQ);
-		int index_id = generate_index_id(parse, def->id, cursor);
+		int index_id = vdbe_emit_new_sec_index_id(parse, def->id,
+							  cursor);
 		sqlVdbeAddOp1(vdbe, OP_Close, cursor);
 		vdbe_emit_create_index(parse, def, index->def, def->id, index_id);
 		sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE);

[tarantool-patches] Re: [PATCH v2 2/4] sql: refactor getNewIid() function

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]


>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 8fb001433..f55f6d800 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -1918,45 +1918,43 @@ sql_drop_foreign_key(struct Parse *parse_context)
>> 	sqlVdbeChangeP5(sqlGetVdbe(parse_context), OPFLAG_NCHANGE);
>> }
>> 
>> -/*
>> - * Generate code to determine next free Iid in the space identified by
>> - * the iSpaceId. Return register number holding the result.
>> +/**
>> + * Generate code to determine next free index id in
>> + * the space identified by the @space_id.
> 
> 1. According to doxygen docs http://www.doxygen.nl/manual/commands.html <http://www.doxygen.nl/manual/commands.html>
> there is no such a command as @space_id. But obviously I know what
> you've implied here. For references to parameters we use '@a' before a
> parameter. Like this:
>    ...
>    * the space identifier by @a space_id.
>    ...

Does anybody even try to generate doxygen doc?
I still believe that doxygen format may be useful only
for external (aka interface) functions. The rest of comments
(especially for static funcs) are seem to be aimed solely at
developers.

> Note that I deliberately used @return instead of @retval, because
> @return has no arguments - it is just a description.
> 
> =================================================================
> 
> commit 9e77985d09e53332d87b323c6ef74b747c418d8e
> Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org>>
> Date:   Thu Mar 28 15:53:24 2019 +0300
> 
>    Review fix

Thx, applied.



[-- Attachment #2: Type: text/html, Size: 15241 bytes --]

[tarantool-patches] Re: [PATCH v2 2/4] sql: refactor getNewIid() function

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [19/03/29 21:18]:
> > 1. According to doxygen docs http://www.doxygen.nl/manual/commands.html <http://www.doxygen.nl/manual/commands.html>
> > there is no such a command as @space_id. But obviously I know what
> > you've implied here. For references to parameters we use '@a' before a
> > parameter. Like this:
> >    ...
> >    * the space identifier by @a space_id.
> >    ...
> 
> Does anybody even try to generate doxygen doc?
> I still believe that doxygen format may be useful only
> for external (aka interface) functions. The rest of comments
> (especially for static funcs) are seem to be aimed solely at
> developers.

True, but functions and comments live for a long time so it's
better to stick to a single style of comments.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] [PATCH v2 3/4] sql: fix error message for improperly created index

[Nikita Pettik]

Table can be created without any indexes (for instance, from Lua-land).
On the other hand, bytecode generated for CREATE INDEX statement
attempts at finding entry in _index space with given space id.
In case it is not found error "wrong space id" is raised. On the other
hand, it is obvious that such message is appeared when table doesn't
have any created indexes yet. Hence, lets fix this message to indicate
that primary key should be created before any secondary indexes.

Closes #3914
---
 src/box/sql/build.c          |  3 +--
 test/sql-tap/index1.test.lua | 19 ++++++++++++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index f55f6d800..20cc346a0 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1946,8 +1946,7 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)
 	sqlVdbeJumpHere(v, seek_adr);
 	sqlVdbeJumpHere(v, seek_adr + 1);
 	sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
-		      sqlMPrintf(parse->db, "Invalid space id: %d", space_id),
-		      P4_DYNAMIC);
+		      "can not add a secondary key before primary", P4_STATIC);
 
 	sqlVdbeJumpHere(v, goto_succ_addr);
 	/* Fetch iid from the row and increment it. */
diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua
index b23e9b39a..5f6706a40 100755
--- a/test/sql-tap/index1.test.lua
+++ b/test/sql-tap/index1.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(70)
+test:plan(72)
 
 --!./tcltestrunner.lua
 -- 2001 September 15
@@ -1016,5 +1016,22 @@ if (0 > 0)
 
 end
 
+test:do_test(
+    "index-22.1.0",
+    function()
+        format = {}
+        format[1] = { name = 'id', type = 'scalar'}
+        format[2] = { name = 'f2', type = 'scalar'}
+        s = box.schema.create_space('T', {format = format})
+    end,
+    {})
+
+test:do_catchsql_test(
+    "alter-8.1.1",
+    [[
+        CREATE UNIQUE INDEX pk ON t("id");
+    ]], {
+        1, "can not add a secondary key before primary"
+    })
 
 test:finish_test()
-- 
2.15.1

[tarantool-patches] Re: [PATCH v2 3/4] sql: fix error message for improperly created index

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/03/28 15:11]:
> Table can be created without any indexes (for instance, from Lua-land).
> On the other hand, bytecode generated for CREATE INDEX statement
> attempts at finding entry in _index space with given space id.
> In case it is not found error "wrong space id" is raised. On the other
> hand, it is obvious that such message is appeared when table doesn't
> have any created indexes yet. Hence, lets fix this message to indicate
> that primary key should be created before any secondary indexes.

We use ER_ALTER_SPACE error code for that in box/alter.cc:

                tnt_raise(ClientError, ER_ALTER_SPACE,
                          space_name(old_space),
                          "can not add a secondary key before primary");

Would it be possible to reuse this logic?


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH v2 3/4] sql: fix error message for improperly created index

[Vladislav Shpilevoy]

Thanks for the patch! See my review fix at the bottom
of the email, and on the branch. In my fixes I've accounted
Kostja's comment about a proper error code.

On 28/03/2019 15:07, Nikita Pettik wrote:
> Table can be created without any indexes (for instance, from Lua-land).
> On the other hand, bytecode generated for CREATE INDEX statement
> attempts at finding entry in _index space with given space id.
> In case it is not found error "wrong space id" is raised. On the other
> hand, it is obvious that such message is appeared when table doesn't
> have any created indexes yet. Hence, lets fix this message to indicate
> that primary key should be created before any secondary indexes.
> 
> Closes #3914
> ---
>  src/box/sql/build.c          |  3 +--
>  test/sql-tap/index1.test.lua | 19 ++++++++++++++++++-
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index f55f6d800..20cc346a0 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1946,8 +1946,7 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)
>  	sqlVdbeJumpHere(v, seek_adr);
>  	sqlVdbeJumpHere(v, seek_adr + 1);
>  	sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
> -		      sqlMPrintf(parse->db, "Invalid space id: %d", space_id),
> -		      P4_DYNAMIC);
> +		      "can not add a secondary key before primary", P4_STATIC);
>  
>  	sqlVdbeJumpHere(v, goto_succ_addr);
>  	/* Fetch iid from the row and increment it. */

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

commit 50adf5f01374a4cd6741c862e23ba902ab651909
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Mar 28 16:03:26 2019 +0300

    Review fix

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 076f62dde..78cd3cee6 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1943,11 +1943,16 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id,
 	sqlVdbeAddOp4Int(v, OP_IdxLT, _index_cursor, 0, key_reg, 1);
 	/* Jump over Halt block. */
 	int goto_succ_addr = sqlVdbeAddOp0(v, OP_Goto);
-	/* Invalid space id handling block starts here. */
+	/*
+	 * Absence of any records in _index for that space is
+	 * handled here.
+	 */
 	sqlVdbeJumpHere(v, seek_adr);
 	sqlVdbeJumpHere(v, seek_adr + 1);
-	sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
-		      "can not add a secondary key before primary", P4_STATIC);
+	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);
 	/* Fetch iid from the row and increment it. */

[tarantool-patches] Re: [PATCH v2 3/4] sql: fix error message for improperly created index

[n.pettik]

> On 28 Mar 2019, at 18:11, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Thanks for the patch! See my review fix at the bottom
> of the email, and on the branch. In my fixes I've accounted
> Kostja's comment about a proper error code.

I guess Kostja meant to remove this check and use means of
on_replace_dd_index() trigger. In other words, generate creation
of secondary index without HALT and index id == 1 to delegate
verification of index correctness to on replace trigger.

Here’s what I did:
(I put this version on separate branch:
https://github.com/tarantool/tarantool/tree/np/gh-3914-fix-create-index-v3)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 78cd3cee6..ef19ac926 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1924,9 +1924,14 @@ sql_drop_foreign_key(struct Parse *parse_context)
  * following:
  *
  * 1 Seek for space id in _index, goto l1 if seeks fails.
- * 2 Goto l2.
- * 3 l1: Halt.
- * 4 l2: Fetch index id from _index record.
+ * 2 Fetch index id from _index record.
+ * 3 Goto l2
+ * 4 l1: Generate iid == 1..
+ * 6 l2: Continue index creation.
+ *
+ * Note that we generate iid == 1 in case of index search on
+ * purpose: it allows on_replace_dd_index() raise correct
+ * error - "can not add a secondary key before primary".
  *
  * @return Register holding a new index id.
  */
@@ -1941,24 +1946,20 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id,
        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 iid_reg = ++parse->nMem;
+       /* Fetch iid from the row and increment it. */
+       sqlVdbeAddOp3(v, OP_Column, _index_cursor, BOX_INDEX_FIELD_ID, iid_reg);
+       sqlVdbeAddOp2(v, OP_AddImm, iid_reg, 1);
        int goto_succ_addr = sqlVdbeAddOp0(v, OP_Goto);
+       sqlVdbeJumpHere(v, seek_adr);
+       sqlVdbeJumpHere(v, seek_adr + 1);
        /*
         * Absence of any records in _index for that space is
-        * handled here.
+        * handled here: to indicate that secondary index can't
+        * be created before primary.
         */
-       sqlVdbeJumpHere(v, seek_adr);
-       sqlVdbeJumpHere(v, seek_adr + 1);
-       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);
-
+       sqlVdbeAddOp2(v, OP_Integer, 1, iid_reg);
        sqlVdbeJumpHere(v, goto_succ_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);
-       sqlVdbeAddOp2(v, OP_AddImm, iid_reg, 1);
        return iid_reg;
 }
 
diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua
index 5f6706a40..63c98e206 100755
--- a/test/sql-tap/index1.test.lua
+++ b/test/sql-tap/index1.test.lua
@@ -1031,7 +1031,7 @@ test:do_catchsql_test(
     [[
         CREATE UNIQUE INDEX pk ON t("id");
     ]], {
-        1, "can not add a secondary key before primary"
+        1, "Can't modify space 'T': can not add a secondary key before primary"
     })

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

[Nikita Pettik]

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
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
+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;
+	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);
+}
+
 /**
  * Add new index to table's indexes list.
  * We follow convention that PK comes first in list.
@@ -2413,9 +2427,23 @@ sql_create_index(struct Parse *parse) {
 				  (void *)space_by_id(BOX_INDEX_ID),
 				  P4_SPACEPTR);
 		sqlVdbeChangeP5(vdbe, OPFLAG_SEEKEQ);
-		int index_id = generate_index_id(parse, def->id, cursor);
+		int index_id;
+		/*
+		 * In case we are creating PRIMARY KEY constraint
+		 * (via ALTER TABLE) we must ensure that table
+		 * doesn't feature any indexes. Otherwise,
+		 * 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);
+		} else {
+			index_id = generate_index_id(parse, def->id, cursor);
+		}
 		sqlVdbeAddOp1(vdbe, OP_Close, cursor);
-		vdbe_emit_create_index(parse, def, index->def, def->id, index_id);
+		vdbe_emit_create_index(parse, def, index->def,
+				       def->id, index_id);
 		sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE);
 		sqlVdbeAddOp0(vdbe, OP_Expire);
 	}
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}
+unique_spec(U) ::= UNIQUE.      { U = SQL_INDEX_TYPE_CONSTRAINT_UNIQUE; }
+unique_spec(U) ::= PRIMARY KEY. { U = SQL_INDEX_TYPE_CONSTRAINT_PK; }
+
 cmd ::= alter_table_start(A) RENAME TO nm(N). {
     rename_entity_def_init(&pParse->rename_entity_def, A, &N);
     sql_alter_table_rename(pParse);
diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
index 3b2eceb98..3e87fbb8b 100755
--- a/test/sql-tap/alter.test.lua
+++ b/test/sql-tap/alter.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(43)
+test:plan(50)
 
 test:do_execsql_test(
     "alter-1.1",
@@ -517,6 +517,61 @@ test:do_catchsql_test(
         -- </alter-7.11>
     })
 
+test:do_test(
+    "alter-8.1.0",
+    function()
+        format = {}
+        format[1] = { name = 'id', type = 'scalar'}
+        format[2] = { name = 'f2', type = 'scalar'}
+        s = box.schema.create_space('T', {format = format})
+    end,
+    {})
+
+test:do_catchsql_test(
+    "alter-8.1.1",
+    [[
+        ALTER TABLE t ADD CONSTRAINT pk PRIMARY KEY("id");
+    ]], {
+        0
+    })
+
+test:do_test(
+    "alter-8.1.2",
+    function()
+        return box.space.T.index[0].id
+    end, 0)
+
+test:do_catchsql_test(
+    "alter-8.2",
+    [[
+        ALTER TABLE t ADD CONSTRAINT pk1 PRIMARY KEY("f2");
+    ]], {
+        1, "multiple primary keys are not allowed"
+    })
+
+test:do_catchsql_test(
+    "alter-8.3.1",
+    [[
+        ALTER TABLE t ADD CONSTRAINT i1 UNIQUE("f2");
+    ]], {
+        0
+    })
+
+test:do_test(
+    "alter-8.3.2",
+    function()
+        i = box.space.T.index[1]
+        return i.id
+    end, 1)
+
+test:do_catchsql_test(
+    "alter-8.4",
+    [[
+        DROP INDEX i1 ON t;
+        DROP INDEX pk ON t;
+    ]], {
+    0
+})
 
 -- Commented due to #2953
 --
diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua
index 5f6706a40..b0e3ece18 100755
--- a/test/sql-tap/index1.test.lua
+++ b/test/sql-tap/index1.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(72)
+test:plan(73)
 
 --!./tcltestrunner.lua
 -- 2001 September 15
@@ -1034,4 +1034,13 @@ test:do_catchsql_test(
         1, "can not add a secondary key before primary"
     })
 
+test:do_catchsql_test(
+    "alter-8.2",
+    [[
+        ALTER TABLE t ADD CONSTRAINT pk PRIMARY KEY("id");
+        CREATE UNIQUE INDEX i ON t("id");
+    ]], {
+    0
+})
+
 test:finish_test()
-- 
2.15.1

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

[Vladislav Shpilevoy]

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; }
 

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

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 3398 bytes --]



> 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.



[-- Attachment #2: Type: text/html, Size: 22963 bytes --]

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

[Vladislav Shpilevoy]

Your diff to the previous commit is ok to me. But probably
in such a case it is worth doing the same for the next commit,
creating a primary index?

I force pushed my review fixes and copy pasted them below. After
that LGTM.

Kirill, note, I *force* pushed. Do not forget to pull the
newest version before push into the master.

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

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 38cd74da4..c7248f734 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1993,30 +1993,6 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id,
 	return iid_reg;
 }
 
-/**
- * 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 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, not_found_addr);
-	int index_id = parse->nMem++;
-	sqlVdbeAddOp2(v, OP_Integer, 0, index_id);
-	return index_id;
-}
-
 /**
  * Add new index to table's indexes list.
  * We follow convention that PK comes first in list.
@@ -2482,8 +2458,8 @@ sql_create_index(struct Parse *parse) {
 		 * we can immediately halt execution of VDBE.
 		 */
 		if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK) {
-			index_id = vdbe_emit_new_pk_index_id(parse, def->id,
-							     cursor);
+			index_id = ++parse->nMem;
+			sqlVdbeAddOp2(vdbe, OP_Integer, 0, index_id);
 		} else {
 			index_id = vdbe_emit_new_sec_index_id(parse, def->id,
 							      cursor);
diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
index 3e87fbb8b..2aba4c4e9 100755
--- a/test/sql-tap/alter.test.lua
+++ b/test/sql-tap/alter.test.lua
@@ -546,7 +546,7 @@ test:do_catchsql_test(
     [[
         ALTER TABLE t ADD CONSTRAINT pk1 PRIMARY KEY("f2");
     ]], {
-        1, "multiple primary keys are not allowed"
+        1, "Duplicate key exists in unique index 'primary' in space '_index'"
     })
 
 test:do_catchsql_test(

[tarantool-patches] Re: [PATCH v2 0/4] Introduce ALTER TABLE ADD CONSTRAINT PK/UNIQUE

[Kirill Yukhin]

Hello,

On 28 Mar 15:07, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-3914-fix-create-index-v2
> Issue: https://github.com/tarantool/tarantool/issues/3914
> 
> These patches haven't been significantly changed since previous
> version. The main change is rebase on fresh 2.1.1

I've checked the patchset into master and 2.1 branch.

--
Regards, Kirill Yukhin