Re: Changed SRF in targetlist handling

On Mon, May 23, 2016 at 4:05 PM, David Fetter <david(at)fetter(dot)org> wrote:

> On Mon, May 23, 2016 at 02:39:54PM -0500, 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.
>
> Yes.
>
> Making SRFs in target lists throw an error is a thing that will be
> pretty straightforward to deal with in extant code bases, whatever
> size of pain in the neck it might be. The line of code that caused
> the error would be very clear, and the fix would be very obvious.
>
> Making their behavior different in some way that throws no warnings is
> guaranteed to cause subtle and hard to track bugs in extant code
> bases.

​I'm advocating that if a presently allowed SRF-in-target-list is allowed
to remain it executes using the same semantics it has today. In all other
cases, including LCM, if the present behavior is undesirable to maintain we
throw an error. I'd hope that such an error can be written in such a way
as to name the offending function or functions.

If the user of a complex query doesn't want to expend the effort to locate
the specific instance of SRF that is in violation they will still have the
option to rewrite all of their uses in that particular query.

David J.