Re: Assert failure of the cross-check for nullingrels

I wrote:
> If they have the same clause_relids, then clearly in its current
> form clause_is_computable_at() cannot distinguish them.  So what
> I now think we should do is have clause_is_computable_at() examine
> their required_relids instead.  Those will be different, by
> construction in deconstruct_distribute_oj_quals(), ensuring that
> we select only one of the group of clones.

Since we're hard up against the beta1 wrap deadline, I went ahead
and pushed the v5 patch.  I doubt that it's perfect yet, but it's
a small change and demonstrably fixes the cases we know about.

As I said in the commit message, the main knock I'd lay on v5
is "why not use required_relids all the time?".  I tried to do
that and soon found that the answer is that we're not maintaining
required_relids very accurately.  I found two causes so far:

1. equivclass.c sometimes generates placeholder constant-true
join clauses, and it's being sloppy about that.  It copies
the required_relids of the original clause, but fails to copy
is_pushed_down, making the clause look like it's been assigned
to the wrong side of the join-clause-vs-filter-clause divide.
I found that we need to copy has_clone/is_clone as well.  The
attached quick-hack patch avoids the bugs, but now I feel like
it was a mistake to not add has_clone/is_clone as full-fledged
arguments of make_restrictinfo.  I'm inclined to change that,
but not right before beta1 when we have no evidence of a reachable
bug.  (Mind you, there might *be* a reachable bug, but ...)

2. When distribute_qual_to_rels forces a qual up to a particular
syntactic level, it applies a relid set that very possibly refers
to rels the clause doesn't actually depend on.  This is problematic
because if the clause gets postponed to above some outer join that
nulls those rels, then it looks like it's being evaluated in an
unsafe location.  I think that when we detect commutability of two
outer joins, we need to adjust the relevant min_xxxhand sets more
thoroughly than we do now.  I've not managed to write a patch
for that yet.  One problem is that if we insist on removing all
unreferenced rels from required_relids, we might end up with a
set that mentions *none* of the RHS and therefore fails to keep
the clause from dropping into the LHS where it must not go.
Not sure about a nice way to handle that.

            regards, tom lane

diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 2db1bf6448..e0077aa1e4 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -1993,20 +1993,24 @@ reconsider_outer_join_clauses(PlannerInfo *root)
             if (reconsider_outer_join_clause(root, ojcinfo, true))
             {
                 RestrictInfo *rinfo = ojcinfo->rinfo;
+                RestrictInfo *ninfo;

                 found = true;
                 /* remove it from the list */
                 root->left_join_clauses =
                     foreach_delete_current(root->left_join_clauses, cell);
                 /* throw back a dummy replacement clause (see notes above) */
-                rinfo = make_restrictinfo(root,
+                ninfo = make_restrictinfo(root,
                                           (Expr *) makeBoolConst(true, false),
-                                          true, /* is_pushed_down */
-                                          false,    /* pseudoconstant */
+                                          rinfo->is_pushed_down,
+                                          false,    /* !pseudoconstant */
                                           0,    /* security_level */
                                           rinfo->required_relids,
                                           rinfo->outer_relids);
-                distribute_restrictinfo_to_rels(root, rinfo);
+                /* copy clone marking, too */
+                ninfo->has_clone = rinfo->has_clone;
+                ninfo->is_clone = rinfo->is_clone;
+                distribute_restrictinfo_to_rels(root, ninfo);
             }
         }

@@ -2018,20 +2022,24 @@ reconsider_outer_join_clauses(PlannerInfo *root)
             if (reconsider_outer_join_clause(root, ojcinfo, false))
             {
                 RestrictInfo *rinfo = ojcinfo->rinfo;
+                RestrictInfo *ninfo;

                 found = true;
                 /* remove it from the list */
                 root->right_join_clauses =
                     foreach_delete_current(root->right_join_clauses, cell);
                 /* throw back a dummy replacement clause (see notes above) */
-                rinfo = make_restrictinfo(root,
+                ninfo = make_restrictinfo(root,
                                           (Expr *) makeBoolConst(true, false),
-                                          true, /* is_pushed_down */
-                                          false,    /* pseudoconstant */
+                                          rinfo->is_pushed_down,
+                                          false,    /* !pseudoconstant */
                                           0,    /* security_level */
                                           rinfo->required_relids,
                                           rinfo->outer_relids);
-                distribute_restrictinfo_to_rels(root, rinfo);
+                /* copy clone marking, too */
+                ninfo->has_clone = rinfo->has_clone;
+                ninfo->is_clone = rinfo->is_clone;
+                distribute_restrictinfo_to_rels(root, ninfo);
             }
         }

@@ -2043,20 +2051,24 @@ reconsider_outer_join_clauses(PlannerInfo *root)
             if (reconsider_full_join_clause(root, ojcinfo))
             {
                 RestrictInfo *rinfo = ojcinfo->rinfo;
+                RestrictInfo *ninfo;

                 found = true;
                 /* remove it from the list */
                 root->full_join_clauses =
                     foreach_delete_current(root->full_join_clauses, cell);
                 /* throw back a dummy replacement clause (see notes above) */
-                rinfo = make_restrictinfo(root,
+                ninfo = make_restrictinfo(root,
                                           (Expr *) makeBoolConst(true, false),
-                                          true, /* is_pushed_down */
-                                          false,    /* pseudoconstant */
+                                          rinfo->is_pushed_down,
+                                          false,    /* !pseudoconstant */
                                           0,    /* security_level */
                                           rinfo->required_relids,
                                           rinfo->outer_relids);
-                distribute_restrictinfo_to_rels(root, rinfo);
+                /* copy clone marking, too */
+                ninfo->has_clone = rinfo->has_clone;
+                ninfo->is_clone = rinfo->is_clone;
+                distribute_restrictinfo_to_rels(root, ninfo);
             }
         }
     } while (found);
diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c
index 760d24bebf..8558c8acd4 100644
--- a/src/backend/optimizer/util/restrictinfo.c
+++ b/src/backend/optimizer/util/restrictinfo.c
@@ -513,8 +513,8 @@ extract_actual_join_clauses(List *restrictinfo_list,
         {
             /* joinquals shouldn't have been marked pseudoconstant */
             Assert(!rinfo->pseudoconstant);
-            Assert(!rinfo_is_constant_true(rinfo));
-            *joinquals = lappend(*joinquals, rinfo->clause);
+            if (!rinfo_is_constant_true(rinfo))
+                *joinquals = lappend(*joinquals, rinfo->clause);
         }
     }
 }