[tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW>

[n.pettik]

> On 13 Aug 2019, at 15:42, Roman Khabibov <roman.habibov at tarantool.org> wrote:
>> On Aug 9, 2019, at 18:15, n.pettik <korablev at tarantool.org> wrote:
>>> On 2 Aug 2019, at 15:52, Roman Khabibov <roman.habibov at tarantool.org> wrote:
>>> 
>>> Check that <CREATE VIEW> hasn't <WITH> after <AS>: "CREATE VIEW v
>>> AS WITH ... SELECT ...". Throw error, if it has.
>> 
>> What is the reason for that? I run example from ticket:
>> 
>> tarantool> \set language sql
>> tarantool> \set delimiter ;
>> 
>> tarantool> CREATE TABLE ts (s1 INT PRIMARY KEY);
>> tarantool> INSERT INTO ts VALUES (1);
>> tarantool> WITH RECURSIVE w AS (
>>>           SELECT s1 FROM ts
>>>           UNION ALL
>>>           SELECT s1+1 FROM w WHERE s1 < 4)
>>>         SELECT * FROM w;
>> tarantool> CREATE VIEW v AS WITH RECURSIVE w AS (
>>>           SELECT s1 FROM ts
>>>           UNION ALL
>>>           SELECT s1+1 FROM w WHERE s1 < 4)
>>>         SELECT * FROM w;
>> - null
>> - Space 'W' does not exist
>> ...
>> 
>> So, it fails at the stage of view creation. Please, ask Peter
>> to provide valid example. Otherwise there’s no problem and
>> issue can be closed.
>> 
> https://github.com/tarantool/tarantool/issues/4149#issuecomment-520390204
> 
>> This looks like a crutch. Please, either find out if it is
>> possible to fix this at parser level (changing grammar),
>> or simply close issue.
> Yes, now it’s one-line fix, but error isn't so detailed.
> But now it's a syntax error, as was required in the ticket.

Still, one can a bit modify query and get old error:

tarantool> CREATE VIEW v AS SELECT * FROM ( WITH RECURSIVE w AS ( SELECT s1 FROM ts UNION ALL SELECT s1+1 FROM w WHERE s1 < 4) SELECT * fROM w);
---
- error: Space 'W' does not exist
…

What is more, if user created view with WITH clause
(see example at the end of letter) and updated tnt to
your branch, user would be forced to use force_recovery option:

2019-08-14 00:30:42.835 [27409] main/102/interactive box.cc:329 E> error applying row: {type: 'INSERT', replica_id: 1, lsn: 19, space_id: 280, index_id: 0, tuple: [514, 1, "V", "memtx", 1, {"sql": "CREATE VIEW v AS WITH v1(id) as (select 1) SELECT * FROM ts", "view": true}, [{"name": "S1", "type": "integer", "is_nullable": true, "nullable_action": "none"}]]}
2019-08-14 00:30:42.835 [27409] main/102/interactive tokenize.c:571 E> ER_SQL_EXECUTE: Failed to execute SQL statement: CREATE VIEW v AS WITH v1(id) as (select 1) SELECT * FROM ts
2019-08-14 00:30:42.835 [27409] main/102/interactive F> can't initialize storage: Failed to execute SQL statement: CREATE VIEW v AS WITH v1(id) as (select 1) SELECT * FROM ts;

This is quite unpleasant.

> commit e311d35bd64422bf6468b28a28057d9045817eca
> Author: Roman Khabibov <roman.habibov at tarantool.org>
> Date:   Mon Jul 29 18:00:34 2019 +0300
> 
>    sql: disallow <WITH> in <CREATE VIEW> select

-> sql: disallow WITH clause in CREATE VIEW statement

>    We don't support queries with the syntax:
>    "CREATE VIEW v AS WITH ... SELECT …"

Strictly speaking, we do support some of such queries.
See example at the end of letter.

>    Throw the syntax error in this case.

Instead of banning CTEs in view’s body, we can fix their creation.
The problem is that during view creation all referenced spaces
are fetched by name from SELECT and their reference counters
are incremented to avoid dangling references. It occurs in
update_view_references(). Obviously, CTE tables are not held
in space cache, ergo error “space doesn’t exist” is raised.
So, what we can do is add graceful check to update_view_references().

>    Closes #4149
> 
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 4c9d95f9c..d9ce239ca 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -399,7 +399,7 @@ ifexists(A) ::= .            {A = 0;}
> ///////////////////// The CREATE VIEW statement /////////////////////////////
> //
> cmd ::= CREATE(X) VIEW ifnotexists(E) nm(Y) eidlist_opt(C)
> -          AS select(S). {
> +          AS selectnowith(S). {
>   if (!pParse->parse_only) {
>     create_view_def_init(&pParse->create_view_def, &Y, &X, C, S, E);
>     pParse->initiateTTrans = true;
> diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua
> index 101f4c3e7..cdba27e04 100755
> --- a/test/sql-tap/view.test.lua
> +++ b/test/sql-tap/view.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
> -test:plan(73)
> +test:plan(76)
> 
> --!./tcltestrunner.lua
> -- 2002 February 26
> @@ -1233,4 +1233,39 @@ test:do_catchsql_test(
>         -- </view-23.8>
>     })
> 
> +-- gh-4149: Check error message for
> +-- "CREATE VIEW v AS WITH ... SELECT ...".
> +test:do_execsql_test(
> +    "view-24.1",
> +    [[
> +        CREATE TABLE ts (s1 INT PRIMARY KEY);
> +        INSERT INTO ts VALUES (1);
> +    ]], {
> +        -- <view-24.1>
> +        -- </view-24.1>
> +    })
> +
> +test:do_catchsql_test(
> +    "view-24.2",
> +    [[
> +        CREATE VIEW v AS WITH RECURSIVE w AS (
> +            SELECT s1 FROM ts
> +            UNION ALL
> +            SELECT s1+1 FROM w WHERE s1 < 4)
> +          SELECT * FROM w;
> +    ]], {
> +        -- <view-24.2>
> +        1,"Keyword 'WITH' is reserved. Please use double quotes if 'WITH' is an identifier."
> +        -- </view-24.2>
> +    })

Look at WITH rule in parse.y:

with(A) ::= . {A = 0;}
with(A) ::= WITH wqlist(W).              { A = W; }
with(A) ::= WITH RECURSIVE wqlist(W).    { A = W; }

So, basically you should also check cases like:

CREATE VIEW v AS WITH v1(id) as (select 1) SELECT * FROM ts;

(It works on master branch, but this query is absolutely meaningless)