From: Tom Lane <tgl@sss.pgh.pa.us>
To: Thomas Munro <thomas.munro@enterprisedb.com>
cc: Robert Haas <robertmhaas@gmail.com>,
        David Rowley <david.rowley@2ndquadrant.com>,
        Bartosz Polnik <bartoszpolnik@gmail.com>,
        PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #15577: Query returns different results when executed
 multiple times
In-reply-to: 
 <CAEepm=1gpbnPQdWEA5TSKwnnAQ0gySTr0+XN+L14dmw2zLSjpA@mail.gmail.com>
References: <15577-ca61ab18904af852@postgresql.org>
 <CAKJS1f8kLijpf-EKHLL7yJ2KJy9-FWNQfXBuNtToVTPkW_Ue+w@mail.gmail.com>
 <CAM37Zev2xUezKc0XZFPfi=BtH+of+fqTAYUCJJuf+4Pc6u4G7g@mail.gmail.com>
 <CAEepm=0yv2qNDM_JfiakqQO3EqYNhQZjNCAX=fK8Cuhz6_RO4Q@mail.gmail.com>
 <CAM37ZetJ-R5De+aTfZV8+BE_KzqbajF3gjv_t1DGsS853aDc0Q@mail.gmail.com>
 <CAEepm=15_MWhhp-Pg6rW1whuXrBTJ-gM=w+2t-Qk-VQHgniG0w@mail.gmail.com>
 <CAM37ZevSHt0pdKhZ_Bu7nDoMMWSDsJaaPUPR_ApowoFida4fcA@mail.gmail.com>
 <CAEepm=1QU7HpJSGhBkfFwKC_wW6fCTADeHymA7ngzszJRM39bw@mail.gmail.com>
 <CAM37Zeu1gv=PswA-C4WeCN13KrrXcD9s1mTdaTuVTKjV=aLoiA@mail.gmail.com>
 <CAEepm=34FwV5W3qoz29_bS1uodKkN1wS-P_9V429JUjjGKwopg@mail.gmail.com>
 <CAM37ZeuWGV_58O8MeH0TfqBiK0JUONPhHs2=YM2MR61K2E=v9Q@mail.gmail.com>
 <CAKJS1f8OZteLVbmKyoTuC7wY=fzZ+jW=gFV=0NoXS0AvfbOZ2g@mail.gmail.com>
 <CAEepm=2MYvOvsA7YY-9kU0JT-2Bn51N-YjGq1mDi4GOeR=0nJA@mail.gmail.com>
 <CAEepm=3s70u7-E9aSdsSq+_S_cm2GnT4Tk3_vC81PDxe=hCFRw@mail.gmail.com>
 <CAM37Zeshup0OkzJ2z=8_jBPrv-=dQZvs+v0HDm!
 KnFf4wLn7jzQ@mail.gmail.com>
 <CAEepm=0eviDVN2n=Dr7FegnJ69w3Z+EaB=CBV5Y9HXLN_HwXKQ@mail.gmail.com>
 <CAEepm=2UuWkK-tUZMK=SqJ8Qn3jX0jTpE=wk5KKSSJB0gYzSXQ@mail.gmail.com>
 <CAKJS1f8zFL8539hpy_gBV3JOcNRqhb1wVVhuVkp_H_E_qJU_iw@mail.gmail.com>
 <CAEepm=0ZqJp2nXRZf8e9ys6Utyu96QVy0=L8Q=-2UoDxwY=smA@mail.gmail.com>
 <18883.1547071723@sss.pgh.pa.us>
 <CA+TgmoZEeHpKYdbksgDXTpE2o=AXOfX08NudPcz_fHYiu84gUg@mail.gmail.com>
 <5725.1547090985@sss.pgh.pa.us> <2887.1547141893@sss.pgh.pa.us>
 <CAEepm=1gpbnPQdWEA5TSKwnnAQ0gySTr0+XN+L14dmw2zLSjpA@mail.gmail.com>
Comments: In-reply-to Thomas Munro <thomas.munro@enterprisedb.com>
	message dated "Fri, 11 Jan 2019 08:17:03 +1300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <13254.1547149893.1@sss.pgh.pa.us>
Date: Thu, 10 Jan 2019 14:51:33 -0500
Message-ID: <13255.1547149893@sss.pgh.pa.us>
Precedence: bulk

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