Re: BUG #17781: Assert in setrefs.c

On Wed, Feb 8, 2023 at 3:02 PM Richard Guo <guofenglinux@gmail.com> wrote:

>
> On Wed, Feb 8, 2023 at 1:52 PM Robins Tharakan <tharakan@gmail.com> wrote:
>
>> Hi Tom,
>>
>> Assuming this persistence is helpful overall, now I see a
>> different SQL can still trigger the same assert().
>>
>> On Wed, 8 Feb 2023 at 09:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > PG Bug reporting form <noreply@postgresql.org> writes:
>> > > This assert() is easily reproducible as of aa69541046@master,
>> although I can
>> > > trace the issue back to last week's commit 2489d76c49.
>> >
>> > Pushed a fix, thanks!
>>
>>
>> TRAP: failed Assert("nrm_match == NRM_SUBSET ?
>> bms_is_subset(phv->phnullingrels, subphv->phnullingrels) : nrm_match
>> == NRM_SUPERSET ? bms_is_subset(subphv->phnullingrels,
>> phv->phnullingrels) : bms_equal(subphv->phnullingrels,
>> phv->phnullingrels)"), File: "setrefs.c", Line: 2845, PID: 803757
>>
>>
>> SQL
>> ===
>> rollback;
>> begin;
>> create table t();
>> SELECT ref_1.definition AS c4
>> FROM t AS sample_1
>>      LEFT JOIN pg_catalog.pg_rules AS ref_1 ON NULL
>> WHERE pg_catalog."user"() IS NOT NULL;
>
>
> I've looked at this a little bit and I think it's a different issue.  It
> seems when we've decided that a left join can be removed, we neglect to
> remove references to the target rel from PlaceHolderVar->phrels in
> remove_rel_from_query.  And it turns out that PlaceHolderVar->phrels is
> used later in build_joinrel_tlist to check whether the PHV actually
> comes from the nullable side of an outer join.  I verified that with the
> following change and it seems can fix this query.
>
> --- a/src/backend/optimizer/plan/analyzejoins.c
> +++ b/src/backend/optimizer/plan/analyzejoins.c
> @@ -429,11 +429,16 @@ remove_rel_from_query(PlannerInfo *root, int relid,
> int ojrelid,
>         }
>         else
>         {
> +           PlaceHolderVar  *phv = phinfo->ph_var;
> +
>             phinfo->ph_eval_at = bms_del_member(phinfo->ph_eval_at, relid);
>             phinfo->ph_eval_at = bms_del_member(phinfo->ph_eval_at,
> ojrelid);
>             Assert(!bms_is_empty(phinfo->ph_eval_at));
>             phinfo->ph_needed = bms_del_member(phinfo->ph_needed, relid);
>             phinfo->ph_needed = bms_del_member(phinfo->ph_needed, ojrelid);
> +
> +           phv->phrels = bms_del_member(phv->phrels, relid);
> +           phv->phrels = bms_del_member(phv->phrels, ojrelid);
>         }
>

Hmm. I begin to wonder if it's better to use phinfo->ph_eval_at instead
in build_joinrel_tlist when we check whether the PHV actually comes from
the nullable side of an outer join.

--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -1095,9 +1095,9 @@ build_joinrel_tlist(PlannerInfo *root, RelOptInfo
*joinrel,
       phv = copyObject(phv);
       /* See comments above to understand this logic */
       if (sjinfo->ojrelid != 0 &&
-          (bms_is_subset(phv->phrels, sjinfo->syn_righthand) ||
+          (bms_is_subset(phinfo->ph_eval_at, sjinfo->syn_righthand) ||
            (sjinfo->jointype == JOIN_FULL &&
-            bms_is_subset(phv->phrels, sjinfo->syn_lefthand))))
+            bms_is_subset(phinfo->ph_eval_at, sjinfo->syn_lefthand))))

Even so it seems we still need to update phv->phrels in
remove_rel_from_query when we remove a left join.  Otherwise it'd be
weird to observe that phrels contains some already-removed relids.

Thanks
Richard