Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtps
 (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1jc56o-0007gU-PF
	for pgsql-hackers@arkaria.postgresql.org; Fri, 22 May 2020 10:40:58 +0000
Received: from localhost ([127.0.0.1] helo=malur.postgresql.org)
	by malur.postgresql.org with esmtp (Exim 4.92)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1jc56H-0002rA-9E
	for pgsql-hackers@arkaria.postgresql.org; Fri, 22 May 2020 10:40:25 +0000
Received: from makus.postgresql.org ([2001:4800:3e1:1::229])
	by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <vik@postgresfriends.org>)
	id 1jc56G-0002r2-S5
	for pgsql-hackers@lists.postgresql.org; Fri, 22 May 2020 10:40:24 +0000
Received: from relay9-d.mail.gandi.net ([217.70.183.199])
	by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <vik@postgresfriends.org>)
	id 1jc569-00028X-BK
	for pgsql-hackers@postgresql.org; Fri, 22 May 2020 10:40:23 +0000
X-Originating-IP: 93.10.30.82
Received: from [172.16.255.146] (82.30.10.93.rev.sfr.net [93.10.30.82])
	(Authenticated sender: vik@postgresfriends.org)
	by relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 86252FF806;
	Fri, 22 May 2020 10:40:13 +0000 (UTC)
Subject: Re: SEARCH and CYCLE clauses
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <db80ceee-6f97-9b4a-8ee8-3ba0c58e5be2@2ndquadrant.com>
 <54e533c5-09e5-7f41-9892-d4e0f7c62598@postgresfriends.org>
 <b267acb8-edc0-31a8-0645-2992d1f0f393@postgresfriends.org>
 <1cfc9079-9595-d167-00e6-5014438cc3e2@2ndquadrant.com>
From: Vik Fearing <vik@postgresfriends.org>
Message-ID: <9ce4d370-048c-1d67-fffb-a86f17120132@postgresfriends.org>
Date: Fri, 22 May 2020 12:40:12 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <1cfc9079-9595-d167-00e6-5014438cc3e2@2ndquadrant.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Precedence: bulk

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