Re: SEARCH and CYCLE clauses

On 5/22/20 11:24 AM, Peter Eisentraut wrote:
> On 2020-05-20 21:28, Vik Fearing wrote:
>> 1)
>> There are some smart quotes in the comments that should be converted to
>> single quotes.
> 
> ok, fixing that
> 
>> 2)
>> This query is an infinite loop, as expected:
>>
>>    with recursive a as (select 1 as b union all select b from a)
>>    table a;
>>
>> But it becomes an error when you add a cycle clause to it:
>>
>>    with recursive a as (select 1 as b union all table a)
>>      cycle b set c to true default false using p
>>    table a;
>>
>>    ERROR:  each UNION query must have the same number of columns
> 
> table a expands to select * from a, and if you have a cycle clause, then
> a has three columns, but the other branch of the union only has one, so
> that won't work anymore, will it?

It seems there was a copy/paste error here.  The first query should have
been the same as the second but without the cycle clause.

It seems strange to me that adding a <search or cycle clause> would
break a previously working query.  I would rather see the * expanded
before adding the new columns.  This is a user's opinion, I don't know
how hard that would be to implement.

>> 3)
>> If I take the same infinite loop query but replace the TABLE syntax with
>> a SELECT and add a cycle clause, it's not an infinite loop anymore.
>>
>>    with recursive a as (select 1 as b union all select b from a)
>>      cycle b set c to true default false using p
>>    table a;
>>
>>   b | c |     p
>> ---+---+-----------
>>   1 | f | {(1)}
>>   1 | t | {(1),(1)}
>> (2 rows)
>>
>> Why does it stop?  It should still be an infinite loop.
> 
> If you specify the cycle clause, then the processing will stop if it
> sees the same row more than once, which it did here.

Yes, this was a misplaced expectation on my part.

>> 4)
>> If I use NULL instead of false, I only get one row back.
>>
>>    with recursive a as (select 1 as b union all select b from a)
>>      cycle b set c to true default false using p
>>    table a;
>>
>>   b | c |   p
>> ---+---+-------
>>   1 |   | {(1)}
>> (1 row)
> 
> If you specify null, then the cycle check expression will always fail,
> so it will abort after the first row.  (We should perhaps prohibit
> specifying null, but see below.)

I would rather make the cycle check expression work with null.

>> 5)
>> I can set both states to the same value.
>>
>>    with recursive a as (select 1 as b union all select b from a)
>>      cycle b set c to true default true using p
>>    table a;
> 
>> This is a direct violation of 7.18 SR 2.b.ii.3 as well as common sense.
>>   BTW, I applaud your decision to violate the other part of that rule and
>> allowing any data type here.
>>
>>
>> 5)
>> The same rule as above says that the value and the default value must be
>> literals but not everything that a human might consider a literal is
>> accepted.  In particular:
>>
>>    with recursive a as (select 1 as b union all select b from a)
>>      cycle b set c to 1 default -1 using p
>>    table a;
>>
>>    ERROR:  syntax error at or near "-"
>>
>> Can we just accept a full a_expr here instead of AexprConst?  Both
>> DEFAULT and USING are fully reserved keywords.
> 
> This is something we need to think about.  If we want to check at parse
> time whether the two values are not the same (and perhaps not null),
> then we either need to restrict the generality of what we can specify,
> or we need to be prepared to do full expression evaluation in the
> parser.  A simple and practical way might be to only allow string and
> boolean literal.  I don't have a strong opinion here.


I'm with Pavel on this one.  Why does it have to be a parse-time error?
 Also, I regularly see people write functions as sort of pauper's
variables, but a function call isn't allowed here.

----

Another bug I found.  If I try to do your regression test as an
autonomous query, I get this:

    with recursive

    graph (f, t, label) as (
        values (1, 2, 'arc 1 -> 2'),
               (1, 3, 'arc 1 -> 3'),
               (2, 3, 'arc 2 -> 3'),
               (1, 4, 'arc 1 -> 4'),
               (4, 5, 'arc 4 -> 5'),
               (5, 1, 'arc 5 -> 1')
    ),

    search_graph (f, t, label) as (
        select * from graph g
        union all
        select g.*
        from graph g, search_graph sg
        where g.f = sg.t
    )
    cycle f, t set is_cycle to true default false using path

    select * from search_graph;

    ERROR:  could not find CTE "graph"

----

As an improvement over the spec, I think the vast majority of people
will be using simple true/false values.  Can we make that optional?

    CYCLE f, t SET is_cycle USING path

would be the same as

    CYCLE f, t SET is_cycle TO true DEFAULT false USING path
-- 
Vik Fearing