public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Heikki Linnakangas <[email protected]>
Cc: Paul Jungwirth <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Inline non-SQL SRFs using SupportRequestSimplify
Date: Fri, 26 Jul 2024 14:58:54 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>

Heikki Linnakangas <[email protected]> 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 having two support requests that
>> did almost exactly the same thing. OTOH my current approach means you'll get an error if you do this:
>> 
>> ```
>> postgres=# select temporal_semijoin('employees', 'id', 'valid_at', 'positions', '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

view thread (19+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Inline non-SQL SRFs using SupportRequestSimplify
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox