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: Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)
Date: 2017-01-18 13:43:24
Message-ID: 12780.1484747004@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I did a review pass over 0001 and 0002. I think the attached updated
version is committable ... except for one thing. The more I look at it,
the more disturbed I am by the behavioral change shown in rangefuncs.out
--- that's the SRF-in-one-arm-of-CASE issue. (The changes in tsrf.out
are fine and as per agreement.) We touched lightly on that point far
upthread, but didn't really resolve it. What's bothering me is that
we're changing, silently, from a reasonably-intuitive behavior to a
completely-not-intuitive one. Since we got a bug report for the previous
less-than-intuitive behavior for such cases, it's inevitable that we'll
get bug reports for this. I think it'd be far better to throw error for
SRF-inside-a-CASE. If we don't, we certainly need to document this,
and I'm not very sure how to explain it clearly.

Upthread we had put COALESCE in the same bucket, but I think that's less
of a problem, because in typical usages the SRF would be in the first
argument and so users wouldn't be expecting conditional evaluation.

Anyway, I've not done anything about that in the attached. What I did do:

* Merge 0001 and 0002. I appreciate you having separated that for my
review, but it doesn't make any sense to commit the parts of 0001 that
you undid in 0002.

* Rename the node types as per yesterday's discussion.

* Require Project to always have an input plan node.

* Obviously, ExecMakeFunctionResultSet can be greatly simplified now
that it need not deal with hasSetArg cases. I saw you'd left that
for later, which is mostly fine, but I did lobotomize it just enough
to throw an error if it gets a set result from an argument. Without
that, we wouldn't really be testing that the planner splits nested
SRFs correctly.

* This bit in ExecProjectSRF was no good:

+ else if (IsA(gstate->arg, FuncExprState) &&
+ ((FuncExpr *) gstate->arg->expr)->funcretset)

because FuncExprState is used for more node types than just FuncExpr;
in particular this would fail (except perhaps by accident) for a
set-returning OpExpr. I chose to fix it by adding a funcReturnsSet
field to FuncExprState and insisting that ExecInitExpr fill that in
immediately, which it can do easily.

* Minor style and comment improvements; fix a couple small oversights
such as missing outfuncs.c support.

* Update the user documentation (didn't address the CASE issue, though).

regards, tom lane

Attachment Content-Type Size
use-project-set-for-tlist-srfs.patch.gz application/x-gzip 21.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Petr Jelinek 2017-01-18 13:46:56 Re: Logical replication existing data copy
Previous Message Stephen Frost 2017-01-18 13:25:44 Re: Re: Clarifying "server starting" messaging in pg_ctl start without --wait