check_srf_call_placement() isn't always setting p_hasTargetSRFs
Hi,
while working on [1]http://archives.postgresql.org/message-id/20170116032952.z2re55hcfhzbkmrm%40alap3.anarazel.de (works, harmless regression check failures aside,
needs cleanup), I noticed $subject. Without fixing EXPR_KIND_VALUES to
set p_hasTargetSRFs the query has Query->hasTargetSRF wrongly set.
Which in turn breaks your planner code, as it's not being triggered
anymore.
Is there a reason not to just set p_hasTargetSRFs once towards the end
of the function, instead of doing so for all the non-error cases? That
fixes INSERT ... VALUES(generate_series()) with your approach for me.
I also noticed that we currently don't actually handle
Query->hasTargetSRF correct when said SRF is in a VALUES() block. As
there the SRFs are't in the targetlist
PlannerInfo *
subquery_planner(PlannerGlobal *glob, Query *parse,
PlannerInfo *parent_root,
bool hasRecursion, double tuple_fraction)
...
/* Constant-folding might have removed all set-returning functions */
if (parse->hasTargetSRFs)
parse->hasTargetSRFs = expression_returns_set((Node *) parse->targetList);
unsets that SRFs exist. I'm not really sure whether that's bad or good
- it right now doesn't cause active problems because of
transformInsertStmt's
/*
* Process INSERT ... VALUES with a single VALUES sublist. We treat
* this case separately for efficiency. The sublist is just computed
* directly as the Query's targetlist, with no VALUES RTE. So it
* works just like a SELECT without any FROM.
*/
optimization.
As we don't support SRFs in any other kind of values (ValuesNext calls
ExecEvalExpr isDone = NULL), that appears to be effectless right now.
But we'd get better errormessage if we made check_srf_call_placement()
error out. I wonder if there should be a seperate expression type for
the INSERT ... VALUES(exactly-one-row); since that behaves quite
differently.
Greetings,
Andres Freund
[1]: http://archives.postgresql.org/message-id/20170116032952.z2re55hcfhzbkmrm%40alap3.anarazel.de
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
Is there a reason not to just set p_hasTargetSRFs once towards the end
of the function, instead of doing so for all the non-error cases?
Yes: it's not supposed to get set when the SRF is in FROM.
I wonder if there should be a seperate expression type for
the INSERT ... VALUES(exactly-one-row); since that behaves quite
differently.
Perhaps. Or maybe we should just use EXPR_KIND_SELECT_TARGET for that?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
Andres Freund <andres@anarazel.de> writes:
I wonder if there should be a seperate expression type for
the INSERT ... VALUES(exactly-one-row); since that behaves quite
differently.
Perhaps. Or maybe we should just use EXPR_KIND_SELECT_TARGET for that?
After looking around, I think we probably better use a different
EXPR_KIND; even if all the functionality is identical, we don't want
ParseExprKindName() to say "SELECT" when we're throwing an error for
INSERT...VALUES.
Also, I noticed that we don't actually allow SRFs in VALUES RTEs:
regression=# select * from (values(1,generate_series(11,13)),(2,0)) v;
ERROR: set-valued function called in context that cannot accept a set
That's because ValuesNext doesn't handle it. I'm not particularly
excited about fixing that, given that it's always been that way and
no one has complained yet. But check_srf_call_placement() is misinformed,
since it thinks the case works.
Will go fix these things.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017-01-16 14:34:58 -0500, Tom Lane wrote:
Will go fix these things.
Thanks!
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers