Re: Executor's internal ParamExecData Params versus EvalPlanQual

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Executor's internal ParamExecData Params versus EvalPlanQual
Date: 2016-09-22 14:11:20
Message-ID: 14896.1474553480@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> The reason this happens is that EvalPlanQualBegin copies down all the
> es_param_exec_vals values from the outer query into the EPQ execution
> state. That's OK for most uses of es_param_exec_vals values, but not
> at all for cteParam Params, which are used as internal communication
> variables within a plan tree.

After further study and caffeine-consumption, I concluded that that's
wrong. There's nothing wrong with reusing the result of a CTE that has
already been scanned by the original query invocation (its output can't
be affected by the new candidate-for-update row). The problem is
that ExecInitCteScan is assuming that when it initializes as a follower,
the tuplestore read pointer it's given by tuplestore_alloc_read_pointer
will be pointing to the start of the tuplestore. That's wrong; the new
read pointer is defined as being a clone of read pointer 0, which is
already at EOF in this scenario. So a simple and localized fix is to
forcibly rewind the new read pointer, as in the attached patch. (This
should have negligible cost as long as the tuplestore hasn't yet spilled
to disk.)

I also considered redefining tuplestore_alloc_read_pointer as creating
a read pointer that points to start-of-tuplestore. The other existing
callers wouldn't care, because they are working with a just-created,
known-empty tuplestore. But there is a risk of breaking third-party
code, so this didn't seem like a back-patchable solution. Also, I think
the reason why tuplestore_alloc_read_pointer was given this behavior is
so that it could succeed even if the tuplestore has already been moved
forward and perhaps had old data truncated off it. With the other
behavior, it would have to have the same failure cases as
tuplestore_rescan.

BTW, I no longer think the recursive-union case is broken; rather, failure
to copy its communication Param would break it, in scenarios where a
WorkTableScan is underneath a SELECT FOR UPDATE that's underneath the
RecursiveUnion. So that's another reason not to mess with the Param
propagation logic.

regards, tom lane

Attachment Content-Type Size
fix-CTE-within-EvalPlanQual.patch text/x-diff 2.9 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Julian Markwort 2016-09-22 14:44:23 [PATCH] pgpassfile connection option
Previous Message Robert Haas 2016-09-22 14:02:18 Re: Parallel sec scan in plpgsql