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

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index a61e35f92d..54ae0379b3 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -35,7 +35,8 @@

 /* local functions */
 static bool join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo);
-static void remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
+static void remove_rel_from_query(PlannerInfo *root, int relid,
+                                  SpecialJoinInfo *removable_sjinfo,
                                   Relids joinrelids);
 static void remove_rel_from_restrictinfo(RestrictInfo *rinfo,
                                          int relid, int ojrelid);
@@ -93,7 +94,7 @@ restart:
         if (sjinfo->ojrelid != 0)
             joinrelids = bms_add_member(joinrelids, sjinfo->ojrelid);

-        remove_rel_from_query(root, innerrelid, sjinfo->ojrelid, joinrelids);
+        remove_rel_from_query(root, innerrelid, sjinfo, joinrelids);

         /* We verify that exactly one reference gets removed from joinlist */
         nremoved = 0;
@@ -331,10 +332,13 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
  * the planner's data structures that will actually be consulted later.
  */
 static void
-remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
+remove_rel_from_query(PlannerInfo *root, int relid,
+                      SpecialJoinInfo *removable_sjinfo,
                       Relids joinrelids)
 {
     RelOptInfo *rel = find_base_rel(root, relid);
+    int            ojrelid = removable_sjinfo->ojrelid;
+    Relids        join_plus_commute;
     List       *joininfos;
     Index        rti;
     ListCell   *l;
@@ -454,8 +458,19 @@ remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
      * outerjoin-delayed quals, which can get marked as requiring the rel in
      * order to force them to be evaluated at or above the join.  We can't
      * just discard them, though.  Only quals that logically belonged to the
-     * outer join being discarded should be removed from the query.
-     *
+     * outer join being discarded should be removed from the query.  However,
+     * we might encounter a qual that is a clone of a deletable qual with some
+     * outer-join relids added (see deconstruct_distribute_oj_quals).  To
+     * ensure we get rid of such clones as well, add the relids of all OJs
+     * commutable with this one to the set we test against for
+     * pushed-down-ness.
+     */
+    join_plus_commute = bms_union(joinrelids,
+                                  removable_sjinfo->commute_below_l);
+    join_plus_commute = bms_add_members(join_plus_commute,
+                                        removable_sjinfo->commute_above_r);
+
+    /*
      * We must make a copy of the rel's old joininfo list before starting the
      * loop, because otherwise remove_join_clause_from_rels would destroy the
      * list while we're scanning it.
@@ -467,7 +482,7 @@ remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,

         remove_join_clause_from_rels(root, rinfo, rinfo->required_relids);

-        if (RINFO_IS_PUSHED_DOWN(rinfo, joinrelids))
+        if (RINFO_IS_PUSHED_DOWN(rinfo, join_plus_commute))
         {
             /*
              * There might be references to relid or ojrelid in the