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

[n.pettik]

> On 16 Aug 2019, at 22:09, Roman Khabibov <roman.habibov at tarantool.org> wrote:
>> On Aug 14, 2019, at 01:10, n.pettik <korablev at tarantool.org> wrote:
>>> 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
>> …
> +test:do_catchsql_test(
> +    "view-24.4",
> +    [[
> +        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);
> +    ]], {
> +        -- <view-24.4>
> +        1,"Failed to create view 'V' with <WITH>"
> +        -- </view-24.4>
> +    })
> 
>> 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().
> If I understand you correctly, we should check <WITH> presence in any select,
> which can meet after <CREATE VIEW>. update_view_references() is called in
> on_drop_view_rollback(), that don't relate to our case. So, I decided to
> check it before update_view_references(), as you can see in diff.

You slightly misunderstood me. I proposed to allow such queries
gracefully handling WITH statement and avoiding references
counter incrementation for CTEs.