Horton

Thread

Commits

  1. Fix check_srf_call_placement() to handle VALUES cases correctly.

  1. check_srf_call_placement() isn't always setting p_hasTargetSRFs

    Andres Freund <andres@anarazel.de> — 2017-01-16T08:15:48Z

    Hi,

while working on [1] (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

  2. Re: check_srf_call_placement() isn't always setting p_hasTargetSRFs

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-16T19:10:24Z

    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

  3. Re: check_srf_call_placement() isn't always setting p_hasTargetSRFs

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-16T19:34:58Z

    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

  4. Re: check_srf_call_placement() isn't always setting p_hasTargetSRFs

    Andres Freund <andres@anarazel.de> — 2017-01-16T21:01:07Z

    On 2017-01-16 14:34:58 -0500, Tom Lane wrote:
> Will go fix these things.

Thanks!

Andres