From: Tom Lane <tgl@sss.pgh.pa.us>
To: Heikki Linnakangas <hlinnaka@iki.fi>
cc: Paul Jungwirth <pj@illuminatedcomputing.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Inline non-SQL SRFs using SupportRequestSimplify
In-reply-to: <ffd7937d-190c-4f09-9418-5345728b45c4@iki.fi>
References: <09de6afa-c33d-4d94-a5cb-afc6cea0d2bb@illuminatedcomputing.com> <ffd7937d-190c-4f09-9418-5345728b45c4@iki.fi>
Comments: In-reply-to Heikki Linnakangas <hlinnaka@iki.fi>
	message dated "Tue, 02 Jul 2024 15:59:45 +0300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1755477.1722020334.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 26 Jul 2024 14:58:54 -0400
Message-ID: <1755478.1722020334@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/1755478.1722020334%40sss.pgh.pa.us>
Precedence: bulk

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 28/06/2024 01:01, Paul Jungwirth wrote:
>> Another approach I considered is using a separate support request, e.g.=
 SupportRequestInlineSRF, and
>> just calling it from inline_set_returning_function. I didn't like havin=
g two support requests that
>> did almost exactly the same thing. OTOH my current approach means you'l=
l get an error if you do this:
>> =

>> ```
>> postgres=3D# select temporal_semijoin('employees', 'id', 'valid_at', 'p=
ositions', 'employee_id',
>> 'valid_at');
>> ERROR:  unrecognized node type: 66
>> ```
>> =

>> I'll look into ways to fix that.

I like this idea, but I like exactly nothing about this implementation.
The right thing is to have a separate SupportRequestInlineSRF request
that is called directly by inline_set_returning_function.  It might be
"almost the same thing" as SupportRequestSimplify, but "almost" only
counts in horseshoes and hand grenades.  In particular, returning a
Query node is simply broken for SupportRequestSimplify (as your
example demonstrates), whereas it's the only correct result for
SupportRequestInlineSRF.

You could imagine keeping it to one support request by adding a
boolean field to the request struct to show which behavior is wanted,
but I think the principal result of that would be to break extensions
that weren't expecting such calls.  The defined mechanism for
extending the SupportRequest protocol is to add new support request
codes, not to whack around the APIs of existing ones.

> I think we should actually add an assertion after the call to the =

> SupportRequestSimplify support function, to check that it returned an =

> Expr node.

Um ... IsA(node, Expr) isn't going to work, and I'm not sure that
it'd be useful to try to enumerate the set of Expr subtypes that
should be allowed there.  But possibly it'd be worth asserting that
it's not a Query, just in case anyone gets confused about the
difference between SupportRequestSimplify and SupportRequestInlineSRF.

It would be good to have an in-core test case for this request type,
but I don't really see any built-in SRFs for which expansion as a
sub-SELECT would be an improvement.

			regards, tom lane