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

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

[Nikita Pettik]

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

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

[n.pettik]

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

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

[Kirill Yukhin]

Hello,

On 19 Mar 23:25, Nikita Pettik 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

Your patch is OK. I've checked it into 2.1 branch.

--
Regards, Kirill Yukhin

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

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/03/20 10:53]:
> +			 "spaces without primary key");

"querying spaces without a primary key"


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