Re: Changed SRF in targetlist handling

On 05/23/2016 12:39 PM, Merlin Moncure wrote:
> On Mon, May 23, 2016 at 2:13 PM, David Fetter <david(at)fetter(dot)org> wrote:
>> On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
>>> On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Andres Freund <andres(at)anarazel(dot)de> writes:
>>>>> discussing executor performance with a number of people at pgcon,
>>>>> several hackers - me included - complained about the additional
>>>>> complexity, both code and runtime, required to handle SRFs in the target
>>>>> list.
>>>>
>>>> Yeah, this has been an annoyance for a long time.
>>>>
>>>>> One idea I circulated was to fix that by interjecting a special executor
>>>>> node to process SRF containing targetlists (reusing Result possibly?).
>>>>> That'd allow to remove the isDone argument from ExecEval*/ExecProject*
>>>>> and get rid of ps_TupFromTlist which is fairly ugly.
>>>>
>>>> Would that not lead to, in effect, duplicating all of execQual.c? The new
>>>> executor node would still have to be prepared to process all expression
>>>> node types.
>>>>
>>>>> Robert suggested - IIRC mentioning previous on-list discussion - to
>>>>> instead rewrite targetlist SRFs into lateral joins. My gut feeling is
>>>>> that that'd be a larger undertaking, with significant semantics changes.
>>>>
>>>> Yes, this was discussed on-list awhile back (I see David found a reference
>>>> already). I think it's feasible, although we'd first have to agree
>>>> whether we want to remain bug-compatible with the old
>>>> least-common-multiple-of-the-periods behavior. I would vote for not,
>>>> but it's certainly a debatable thing.
>>>
>>> +1 on removing LCM.
>>
>> As a green field project, that would make total sense. As a thing
>> decades in, it's not clear to me that that would break less stuff or
>> break it worse than simply disallowing SRFs in the target list, which
>> has been rejected on bugward-compatibility grounds. I suspect it
>> would be even worse because disallowing SRFs in target lists would at
>> least be obvious and localized when it broke code.
>
> If I'm reading this correctly, it sounds to me like you are making the
> case that removing target list SRF completely would somehow cause less
> breakage than say, rewriting it to a LATERAL based implementation for
> example. With more than a little forbearance, let's just say I don't
> agree.

I'm not necessarily saying that we should totally remove target list
SRFs, but I will point out it has been deprecated ever since SRFs were
first introduced:

http://www.postgresql.org/docs/7.3/static/xfunc-sql.html

"Currently, functions returning sets may also be called in the target
list of a SELECT query. For each row that the SELECT generates by
itself, the function returning set is invoked, and an output row is
generated for each element of the function's result set. Note,
however, that this capability is deprecated and may be removed in
future releases."

I would be in favor of rewriting it to a LATERAL, but that would not be
backwards compatible entirely either IIUC.

I'll also note that, unless I missed something, we also have to consider
that the capability to pipeline results is still only available in the
target list.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development