Re: ERROR: no relation entry for relid 6

Richard Guo <guofenglinux@gmail.com> writes:
> On Wed, May 24, 2023 at 2:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wonder if we could do something involving recording the
>> rinfo_serial numbers of all the clauses extracted from a particular
>> syntactic join level (we could keep this in a bitmapset attached
>> to each SpecialJoinInfo, perhaps) and then filtering the joinclauses
>> on the basis of serial numbers instead of required_relids.

> I think this is a better way to fix the issue.  I went ahead and drafted
> a patch as attached.  But I doubt that the collecting of rinfo_serial
> numbers is thorough in the patch.  Should we also collect the
> rinfo_serial of quals generated in reconsider_outer_join_clauses()?

Not sure.  However, I thought of a possible fix that doesn't require
so much new mechanism: we could consider potentially-commuted outer
joins as part of the relid set that's okay to discard, as in the
attached.  This is still relying on RINFO_IS_PUSHED_DOWN, which I
continue to feel is not quite the right thing here, but on the other
hand that logic survived for years without trouble.  What broke it
was addition of outer-join relids to the mix.  I briefly considered
proposing that we simply ignore all outer-join relids in the test that
decides whether to keep or discard a joinqual, but this way seems at
least slightly more principled.

A couple of notes:

1. The test case you give upthread only needs to ignore commute_below_l,
that is it still passes without the lines

+    join_plus_commute = bms_add_members(join_plus_commute,
+                                        removable_sjinfo->commute_above_r);

Based on what deconstruct_distribute_oj_quals is doing, it seems
likely to me that there are cases that require ignoring
commute_above_r, but I've not tried to devise one.  It'd be good to
have one to include in the commit, if we can find one.

2. We could go a little further in refactoring, specifically the
computation of joinrelids could be moved into remove_rel_from_query,
since remove_useless_joins itself doesn't need it.  Not sure if
this'd be an improvement or not.  Certainly it'd be a loser if
remove_useless_joins grew a reason to need the value; but I can't
foresee a reason for that to happen --- remove_rel_from_query is
where basically all the work is going on.

3. I called the parameter removable_sjinfo because sjinfo is already
used within another loop, leading to a shadowed-parameter warning.
In a green field we'd probably have called the parameter sjinfo
and used another name for the loop's local variable, but that would
make the patch bulkier without adding anything.  Haven't decided
whether to rename before commit or leave it as-is.

			regards, tom lane