[tarantool-patches] Re: [PATCH v2 4/4] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Mar 28 18:11:54 MSK 2019
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 at 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,
+ ¬_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,
+ ¬_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; }
More information about the Tarantool-patches
mailing list