[tarantool-patches] Re: [PATCH] sql: make spaces without PK illegal in queries

["n.pettik" <korablev@tarantool.org>]

> On 19 Mar 2019, at 23:25, Nikita Pettik <korablev@tarantool.org> wrote:
> 
> Our SQL codebase was inherited from SQLite, where each table must have
> at least one index - primary (if no explicit PK declared, one based on
> rowid is implicitly created). In Tarantool spaces can exists without
> indexes. The only existing restriction is that they can't contain any
> data. Hence, even very basic queries fail with assertion/seagfault if
> they are applied to spaces with no indexes. Situation when space turns
> out to remain without PK is quite common due to the absence of
> transactional DDL: for instance, space drop procedure consists of
> several steps including dropping all indexes; space itself is dropped at
> the very end. Thus, if a sequence of queries is interrupted by drop
> space procedure and one is not finished, the rest of queries will
> operate on space with no indexes.
> 
> As a solution to this problem (at least temporary) now we disallow
> query processing involving spaces without PK except for views.
> 
> Closes #3780
> ---
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-3780-dissalow-spaces-without-pk
> Issue: https://github.com/tarantool/tarantool/issues/3780
> 
> src/box/sql/delete.c          |  7 +++++
> test/sql/errinj.result        | 48 ++++++++++++++++++++++++++++++++++
> test/sql/errinj.test.lua      | 22 ++++++++++++++++
> test/sql/no-pk-space.result   | 61 +++++++++++++++++++++++++++++++++++++++++++
> test/sql/no-pk-space.test.lua | 24 +++++++++++++++++
> 5 files changed, 162 insertions(+)
> create mode 100644 test/sql/no-pk-space.result
> create mode 100644 test/sql/no-pk-space.test.lua
> 
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 5170c7f59..2b5c5014e 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -54,6 +54,13 @@ sql_lookup_space(struct Parse *parse, struct SrcList_item *space_name)
> 		parse->nErr++;
> 		return NULL;
> 	}
> +	if (space->index_count == 0 && !space->def->opts.is_view) {
> +		diag_set(ClientError, ER_UNSUPPORTED, "SQL",
> +			 "spaces without primary key");
> +		parse->rc = SQL_TARANTOOL_ERROR;
> +		parse->nErr++;
> +		return NULL;
> +	}

After rebase to very fresh 2.1:

diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 5b8f11967..f4d0334f4 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -56,8 +56,7 @@ sql_lookup_space(struct Parse *parse, struct SrcList_item *space_name)
        if (space->index_count == 0 && !space->def->opts.is_view) {
                diag_set(ClientError, ER_UNSUPPORTED, "SQL",
                         "spaces without primary key");
-               parse->rc = SQL_TARANTOOL_ERROR;
-               parse->nErr++;
+               parse->is_aborted = true;
                return NULL;
        }
        space_name->space = space;

> 	space_name->space = space;
> 	if (sqlIndexedByLookup(parse, space_name) != 0)
> 		space = NULL;
> diff --git a/test/sql/errinj.result b/test/sql/errinj.result
> index 49e71ff06..a1e7cc4a3 100644
> --- a/test/sql/errinj.result
> +++ b/test/sql/errinj.result
> @@ -340,3 +340,51 @@ errinj.set("ERRINJ_WAL_IO", false)
> box.sql.execute("DROP TABLE t3;")
> ---
> ...
> +-- gh-3780: space without PK raises error if
> +-- it is used in SQL queries.
> +--
> +errinj = box.error.injection
> +---
> +...
> +fiber = require('fiber')
> +---
> +...
> +box.sql.execute("CREATE TABLE t (id INT PRIMARY KEY);")
> +---
> +...
> +box.sql.execute("INSERT INTO t VALUES (1);")
> +---
> +...
> +errinj.set("ERRINJ_WAL_DELAY", true)
> +---
> +- ok
> +...
> +-- DROP TABLE consists of several steps: firstly indexes
> +-- are deleted, then space itself. Lets make sure that if
> +-- first part of drop is successfully finished, but resulted
> +-- in yield, all operations on space will be blocked due to
> +-- absence of primary key.
> +--
> +function drop_table_yield() box.sql.execute("DROP TABLE t;") end
> +---
> +...
> +f = fiber.create(drop_table_yield)
> +---
> +...
> +box.sql.execute("SELECT * FROM t;")
> +---
> +- error: SQL does not support spaces without primary key
> +...
> +box.sql.execute("INSERT INTO t VALUES (2);")
> +---
> +- error: SQL does not support spaces without primary key
> +...
> +box.sql.execute("UPDATE t SET id = 2;")
> +---
> +- error: SQL does not support spaces without primary key
> +...
> +-- Finish drop space.
> +errinj.set("ERRINJ_WAL_DELAY", false)
> +---
> +- ok
> +...
> diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
> index 80c8fde26..d8833feb4 100644
> --- a/test/sql/errinj.test.lua
> +++ b/test/sql/errinj.test.lua
> @@ -117,3 +117,25 @@ box.sql.execute("ALTER TABLE t3 DROP CONSTRAINT fk1;")
> box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
> errinj.set("ERRINJ_WAL_IO", false)
> box.sql.execute("DROP TABLE t3;")
> +
> +-- gh-3780: space without PK raises error if
> +-- it is used in SQL queries.
> +--
> +errinj = box.error.injection
> +fiber = require('fiber')
> +box.sql.execute("CREATE TABLE t (id INT PRIMARY KEY);")
> +box.sql.execute("INSERT INTO t VALUES (1);")
> +errinj.set("ERRINJ_WAL_DELAY", true)
> +-- DROP TABLE consists of several steps: firstly indexes
> +-- are deleted, then space itself. Lets make sure that if
> +-- first part of drop is successfully finished, but resulted
> +-- in yield, all operations on space will be blocked due to
> +-- absence of primary key.
> +--
> +function drop_table_yield() box.sql.execute("DROP TABLE t;") end
> +f = fiber.create(drop_table_yield)
> +box.sql.execute("SELECT * FROM t;")
> +box.sql.execute("INSERT INTO t VALUES (2);")
> +box.sql.execute("UPDATE t SET id = 2;")
> +-- Finish drop space.
> +errinj.set("ERRINJ_WAL_DELAY", false)
> diff --git a/test/sql/no-pk-space.result b/test/sql/no-pk-space.result
> new file mode 100644
> index 000000000..1d57d1687
> --- /dev/null
> +++ b/test/sql/no-pk-space.result
> @@ -0,0 +1,61 @@
> +test_run = require('test_run').new()
> +---
> +...
> +engine = test_run:get_cfg('engine')
> +---
> +...
> +box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
> +---
> +...
> +format = {}
> +---
> +...
> +format[1] = {'id', 'integer'}
> +---
> +...
> +s = box.schema.create_space('test', {format = format})
> +---
> +...
> +box.sql.execute("SELECT * FROM \"test\";")
> +---
> +- error: SQL does not support spaces without primary key
> +...
> +box.sql.execute("INSERT INTO \"test\" VALUES (1);")
> +---
> +- error: SQL does not support spaces without primary key
> +...
> +box.sql.execute("DELETE FROM \"test\";")
> +---
> +- error: SQL does not support spaces without primary key
> +...
> +box.sql.execute("UPDATE \"test\" SET id = 3;")
> +---
> +- error: SQL does not support spaces without primary key
> +...
> +s:drop()
> +---
> +...
> +-- Notorious artefact: check of view referencing counter occurs
> +-- after drop of indexes. So, if space:drop() fails due to being
> +-- referenced by a view, space becomes unusable in SQL terms.
> +--
> +box.sql.execute("CREATE TABLE t1 (id INT PRIMARY KEY);")
> +---
> +...
> +box.sql.execute("CREATE VIEW v1 AS SELECT * FROM t1;")
> +---
> +...
> +box.space.T1:drop()
> +---
> +- error: 'Can''t drop space ''T1'': other views depend on this space'
> +...
> +box.sql.execute("SELECT * FROM v1;")
> +---
> +- error: SQL does not support spaces without primary key
> +...
> +box.space.V1:drop()
> +---
> +...
> +box.space.T1:drop()
> +---
> +...
> diff --git a/test/sql/no-pk-space.test.lua b/test/sql/no-pk-space.test.lua
> new file mode 100644
> index 000000000..2828777f7
> --- /dev/null
> +++ b/test/sql/no-pk-space.test.lua
> @@ -0,0 +1,24 @@
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')
> +box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
> +
> +format = {}
> +format[1] = {'id', 'integer'}
> +s = box.schema.create_space('test', {format = format})
> +box.sql.execute("SELECT * FROM \"test\";")
> +box.sql.execute("INSERT INTO \"test\" VALUES (1);")
> +box.sql.execute("DELETE FROM \"test\";")
> +box.sql.execute("UPDATE \"test\" SET id = 3;")
> +
> +s:drop()
> +
> +-- Notorious artefact: check of view referencing counter occurs
> +-- after drop of indexes. So, if space:drop() fails due to being
> +-- referenced by a view, space becomes unusable in SQL terms.
> +--
> +box.sql.execute("CREATE TABLE t1 (id INT PRIMARY KEY);")
> +box.sql.execute("CREATE VIEW v1 AS SELECT * FROM t1;")
> +box.space.T1:drop()
> +box.sql.execute("SELECT * FROM v1;")
> +box.space.V1:drop()
> +box.space.T1:drop()
> -- 
> 2.15.1
> 
>