Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)
Date: 2016-09-15 19:23:58
Message-ID: 10954.1473967438@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Andres Freund <andres(at)anarazel(dot)de> writes:
> 0003-Avoid-materializing-SRFs-in-the-FROM-list.patch
> To avoid performance regressions from moving SRFM_ValuePerCall SRFs to
> ROWS FROM, nodeFunctionscan.c needs to support not materializing
> output.

I looked over this patch a bit.

> In my present patch I've *ripped out* the support for materialization
> in nodeFunctionscan.c entirely. That means that rescans referencing
> volatile functions can change their behaviour (if a function is
> rescanned, without having it's parameters changed), and that native
> backward scan support is gone. I don't think that's actually an issue.

I think you are wrong on this not being an issue: it is critical that
rescan deliver the same results as before, else for example having a
function RTE on the inside of a nestloop will give nonsensical/broken
results. I think what we'll have to do is allow the optimization of
skipping the tuplestore only when the function is declared nonvolatile.
(If it is, and it nonetheless gives different results on rescan, it's not
our fault if joins give haywire answers.) I'm okay with not supporting
backward scan, but wrong answers during rescan is a different animal
entirely.

Moreover, I think we'd all agreed that this effort needs to avoid any
not-absolutely-necessary semantics changes. This one is not only not
necessary, but it would result in subtle hard-to-detect breakage.

It's conceivable that we could allow the executor to be broken this way
and have the planner fix it by inserting a Material node when joining.
But I think it would be messy and would probably not perform as well as
an internal tuplestore --- for one thing, because the planner can't know
whether the function would return a tuplestore, making the external
materialization redundant.

Another idea is that we could extend the set of ExecInitNode flags
(EXEC_FLAG_REWIND etc) to tell child nodes whether they need to implement
rescan correctly in this sense; if they are not RHS children of nestloops,
and maybe one or two other cases, they don't. That would give another
route by which nodeFunctionscan could decide that it can skip
materialization in common cases.

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2016-09-15 19:26:16 Re: PATCH: Keep one postmaster monitoring pipe per process
Previous Message Peter Geoghegan 2016-09-15 19:12:43 Re: Tuplesort merge pre-reading