Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Fri, Jan 11, 2019 at 6:38 AM Tom Lane <tgl@sss.pgh.pa.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
