Re: BUG #15577: Query returns different results when executed multiple times

Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> writes:
> On Fri, Jan 11, 2019 at 6:38 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> At this point assign_nestloop_param_var and
>> assign_nestloop_param_placeholdervar are dead code, and there's a bunch
>> of incorrect comments in subselect.c, and I really need to refactor
>> the division of labor between createplan.c and subselect.c (for one
>> thing, this is an abuse of the documented purpose of
>> SS_make_initplan_output_param). But functionally I think it does the
>> right thing. Please test and verify that you no longer see the race
>> condition.

> I no longer see it here.

Thanks for checking!

I'm having some difficulty choosing what to do refactoring-wise.
There are a couple of conflicting considerations:

* Currently, subselect.c is in charge of assigning PARAM_EXEC slots;
in particular, nothing else touches root->glob->paramExecTypes.
I'm kind of loath to give that up.

* On the other hand, root->curOuterParams is currently only manipulated
by createplan.c, and if we could keep that as a local data structure,
that'd be nice too.

* However, if we stick to both of those constraints then we're forced
into more or less what the POC patch does. We could provide another
subselect.c function, say SS_make_nestloop_param, which'd just wrap
generate_new_param the same as SS_make_initplan_output_param, but
we still have a pretty weird division of labor IMO.

The fundamental issue here is that it's now going to be the state of the
curOuterParams list that determines whether a new PARAM_EXEC slot is
needed. Really that list serves the same sort of purpose as the
root->plan_params list, but its entries have very different lifespans than
the entries in plan_params. So there's a reasonable case to be made that
we should put management of curOuterParams into subselect.c, except that
(a) it's a bit far afield from sub-selects, and (b) I'm not sure how
completely it could be decoupled from createplan.c.

If this were a HEAD-only patch I'd be tempted to do like Alvaro just did
and move all the PARAM_EXEC assignment logic and root->plan_params
and root->curOuterParams manipulation into a new file, say
optimizer/util/paramassign.c. But that would be a little invasive
for a back-patch.

Thoughts?

regards, tom lane