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

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

Thanks for the patch! See 3 comments below.

On 09/01/2019 15:13, 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
> Follow-up #3914
> ---
>   src/box/sql/build.c         | 29 +++++++++++++++++++++--
>   src/box/sql/parse.y         | 15 ++++++++++++
>   test/sql-tap/alter.test.lua | 58 ++++++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 99 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 1d7b6c827..0314be957 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2083,6 +2083,19 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)
>   	return iid_reg;
>   }
>   
> +static void
> +pk_check_existence(struct Parse *parse, uint32_t space_id, int cursor)

1. It is worth mentioning in a comment that the cursor is opened on _index
space. Or rename it to _index_cursor. It do not mind if in such a case you
omit a comment.

> +{
> +	struct Vdbe *v = sqlite3GetVdbe(parse);
> +	int tmp_reg = ++parse->nMem;

2. Why not to use a truly temp register? As I know, we have facilities for
it. Parse.nMem are not temporary. Parse.nTempReg - is.

> +	sqlite3VdbeAddOp2(v, OP_Integer, space_id, tmp_reg);
> +	int found_addr = sqlite3VdbeAddOp4Int(v, OP_NotFound, cursor, 0,
> +					     tmp_reg, 1);
> +	sqlite3VdbeAddOp4(v, OP_Halt, SQLITE_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
> +			  "multiple primary keys are not allowed", P4_STATIC);
> +	sqlite3VdbeJumpHere(v, found_addr);
> +}
> +
>   /**
>    * Add new index to table's indexes list.
>    * We follow convention that PK comes first in list.
> diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
> index 1aad555c0..925753749 100755
> --- a/test/sql-tap/alter.test.lua
> +++ b/test/sql-tap/alter.test.lua
> @@ -517,6 +517,62 @@ 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()
> +        i = box.space.T.index[0]
> +        return i.id

3. Why not 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
>   --
>