[tarantool-patches] Re: [PATCH v1 1/1] sql: move space existence check to VDBE

[n.pettik]

> On 8 May 2019, at 20:02, imeevma at tarantool.org wrote:
> 
> Before this patch, the existence of space was checked for the
> CREATE TABLE or CREATE VIEW statements during the parsing. If the
> space already exists, an error has been set and cleanup is
> performed. But, if the statement contained 'IF NOT EXIST', the
> cleanup was performed, but the error was not set.

-> Meanwhile, create_foreign_key() assumes that if
create_table_def->new_space is NULL, then we are
dealing with ALTER TABLE statement. This in turn false,
since ctd->new_space is nullified also in case of already
existing table.

> This causes an
> error when creating a foreign key during space creation.

Not error, but segmentation fault.

> This patch moves this check to VDBE. Thus, the parsing should now
> be properly closed before performing this check.

-> parsing now is always processed till the end as in case space
doesn’t exist.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 6051a25..a25479e 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> 
> @@ -1143,6 +1133,23 @@ sqlEndTable(struct Parse *pParse)
> 	struct Vdbe *v = sqlGetVdbe(pParse);
> 	if (NEVER(v == 0))
> 		return;
> +	const char *space_name =
> +		sql_name_from_token(db, &pParse->create_table_def.base.name);

Why not using new_space->def->name ?

> +	char *name_copy = sqlDbStrDup(db, space_name);
> +	if (name_copy == NULL)
> +		goto cleanup;
> +	int name_reg = ++pParse->nMem;
> +	sqlVdbeAddOp4(pParse->pVdbe, OP_String8, 0, name_reg, 0, name_copy,
> +		      P4_DYNAMIC);

In vdbe_emit_space_create() name of space is duplicated as well,
so to avoid processing copy twice we can pass register holding the
name of space to vdbe_emut_space_create().

Consider this diff:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a25479edc..61f4f4127 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -833,7 +833,7 @@ error:
  */
 static void
 vdbe_emit_space_create(struct Parse *pParse, int space_id_reg,
-                      struct space *space)
+                      int space_name_reg, struct space *space)
 {
        Vdbe *v = sqlGetVdbe(pParse);
        int iFirstCol = ++pParse->nMem;
@@ -862,9 +862,7 @@ vdbe_emit_space_create(struct Parse *pParse, int space_id_reg,
        sqlVdbeAddOp2(v, OP_SCopy, space_id_reg, iFirstCol /* spaceId */ );
        sqlVdbeAddOp2(v, OP_Integer, effective_user()->uid,
                          iFirstCol + 1 /* owner */ );
-       sqlVdbeAddOp4(v, OP_String8, 0, iFirstCol + 2 /* name */ , 0,
-                         sqlDbStrDup(pParse->db, space->def->name),
-                         P4_DYNAMIC);
+       sqlVdbeAddOp2(v, OP_SCopy, space_name_reg, iFirstCol + 2);
        sqlVdbeAddOp4(v, OP_String8, 0, iFirstCol + 3 /* engine */ , 0,
                          sqlDbStrDup(pParse->db, space->def->engine_name),
                          P4_DYNAMIC);
@@ -1133,16 +1131,20 @@ sqlEndTable(struct Parse *pParse)
        struct Vdbe *v = sqlGetVdbe(pParse);
        if (NEVER(v == 0))
                return;
-       const char *space_name =
-               sql_name_from_token(db, &pParse->create_table_def.base.name);
-       char *name_copy = sqlDbStrDup(db, space_name);
-       if (name_copy == NULL)
+       /*
+        * Firstly, check if space with given name already exists.
+        * In case IF NOT EXISTS clause is specified and table
+        * exists, we will silently halt VDBE execution.
+        */
+       char *space_name_copy =
+               sqlDbStrDup(db, new_space->def->name);
+       if (space_name_copy == NULL)
                goto cleanup;
        int name_reg = ++pParse->nMem;
-       sqlVdbeAddOp4(pParse->pVdbe, OP_String8, 0, name_reg, 0, name_copy,
+       sqlVdbeAddOp4(pParse->pVdbe, OP_String8, 0, name_reg, 0, space_name_copy,
                      P4_DYNAMIC);
        const char *error_msg =
-               tt_sprintf(tnt_errcode_desc(ER_SPACE_EXISTS), space_name);
+               tt_sprintf(tnt_errcode_desc(ER_SPACE_EXISTS), space_name_copy);
        bool no_err = pParse->create_table_def.base.if_not_exist;
        if (vdbe_emit_halt_with_presence_test(pParse, BOX_SPACE_ID, 2,
                                              name_reg, 1, ER_SPACE_EXISTS,
@@ -1151,7 +1153,7 @@ sqlEndTable(struct Parse *pParse)
                goto cleanup;
 
        int reg_space_id = getNewSpaceId(pParse);
-       vdbe_emit_space_create(pParse, reg_space_id, new_space);
+       vdbe_emit_space_create(pParse, reg_space_id, name_reg, new_space);
        for (uint32_t i = 0; i < new_space->index_count; ++i) {
                struct index *idx = new_space->index[i];
                vdbe_emit_create_index(pParse, new_space->def, idx->def,
@@ -1309,7 +1311,7 @@ sql_create_view(struct Parse *parse_context)
                goto create_view_fail;
 
        vdbe_emit_space_create(parse_context, getNewSpaceId(parse_context),
-                              space);
+                              name_reg, space);

> diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
> index 5b793c0..7881cff 100755
> --- a/test/sql-tap/table.test.lua
> +++ b/test/sql-tap/table.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
> -test:plan(77)
> +test:plan(79)
> 
> --!./tcltestrunner.lua
> -- 2001 September 15
> 
> @@ -1442,4 +1442,30 @@ test:do_execsql_test(
> 
>         -- </check-23.cleanup>
>     })
> +
> +--
> +-- gh-4196: Error creating table twice.

-> Segmentation fault happened when IF NOT EXISTS
clause was specified during creation of space featuring 
FK constraints.

> +--
> +test:do_execsql_test(

I’d make this test be catchsql to indicate that it should
test that no error is raised.

> +	"table-24.1",
> +	[[
> +		CREATE TABLE IF NOT EXISTS a1 (i INT PRIMARY KEY);
> +		CREATE TABLE IF NOT EXISTS b1 (i INT PRIMARY KEY, j INT, CONSTRAINT aa FOREIGN KEY(j) REFERENCES a1(i));
> +		CREATE TABLE IF NOT EXISTS b1 (i INT PRIMARY KEY, j INT, CONSTRAINT aa FOREIGN KEY(j) REFERENCES a1(i));
> +	]], {
> +		-- <table-24.1>
> +		-- </table-24.1>
> +	})
> +