Skip site navigation (1) Skip section navigation (2)

check_srf_call_placement() isn't always setting p_hasTargetSRFs

From: Andres Freund <andres(at)anarazel(dot)de>
To: pgsql-hackers(at)postgresql(dot)org, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: check_srf_call_placement() isn't always setting p_hasTargetSRFs
Date: 2017-01-16 08:15:48
Message-ID: 20170116081548.zg63zltblwimpfgp@alap3.anarazel.de (view raw, whole thread or download thread mbox)
Thread:
Lists: pgsql-hackers
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


Responses

pgsql-hackers by date

Next:From: Andres FreundDate: 2017-01-16 08:33:16
Subject: Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)
Previous:From: Ashutosh BapatDate: 2017-01-16 06:08:33
Subject: Re: Parallel Append implementation

Privacy Policy | About PostgreSQL
Copyright © 1996-2017 The PostgreSQL Global Development Group