Killing off removed rels properly - Mailing list pgsql-hackers

Outer-join removal does this:

    /*
     * Mark the rel as "dead" to show it is no longer part of the join tree.
     * (Removing it from the baserel array altogether seems too risky.)
     */
    rel->reloptkind = RELOPT_DEADREL;

which apparently I thought was a good idea in 2010 (cf b78f6264e),
but looking at it now it just seems like an invitation to fail to
detect bugs.  We've had a couple of recent reports of indxpath.c
failing like this:

Program terminated with signal SIGABRT, Aborted.
#0  __GI_raise (sig=sig(at)entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007f0b57bc6859 in __GI_abort () at abort.c:79
#2  0x0000555ec56d3ff3 in ExceptionalCondition (conditionName=0x555ec5887ff0
"outer_rel->rows > 0", fileName=0x555ec5887f2c "indxpath.c",
lineNumber=1909) at assert.c:66
#3  0x0000555ec538a67a in get_loop_count (root=0x555ec5f72680, cur_relid=3,
outer_relids=0x555ec5f93960) at indxpath.c:1909
#4  0x0000555ec5388b5e in build_index_paths (root=0x555ec5f72680,
rel=0x555ec5f8f648, index=0x555ec5f8ca90, clauses=0x7fffeea57480,
useful_predicate=false, scantype=ST_BITMAPSCAN, skip_nonnative_saop=0x0,
skip_lower_saop=0x0) at indxpath.c:957

This is pretty impenetrable at first glance, but what it boils
down to is something accessing a "dead" rel and taking its contents
at face value.  Fortunately the contents triggered an assertion,
but that hardly seems like something to count on to detect bugs.

I think it's time to clean this up by removing the rel from the
planner data structures altogether.  The attached passes check-world,
and if it does trigger any problems I would say that's a clear
sign of bugs elsewhere.

            regards, tom lane

diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index de335fdb4d..9132ce235f 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -729,8 +729,7 @@ get_eclass_for_sort_expr(PlannerInfo *root,
                 continue;
             }

-            Assert(rel->reloptkind == RELOPT_BASEREL ||
-                   rel->reloptkind == RELOPT_DEADREL);
+            Assert(rel->reloptkind == RELOPT_BASEREL);

             rel->eclass_indexes = bms_add_member(rel->eclass_indexes,
                                                  ec_index);
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 7fd11df9af..0dfefd71f2 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -331,12 +331,6 @@ remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
     Index        rti;
     ListCell   *l;

-    /*
-     * Mark the rel as "dead" to show it is no longer part of the join tree.
-     * (Removing it from the baserel array altogether seems too risky.)
-     */
-    rel->reloptkind = RELOPT_DEADREL;
-
     /*
      * Remove references to the rel from other baserels' attr_needed arrays.
      */
@@ -487,6 +481,16 @@ remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
      * There may be references to the rel in root->fkey_list, but if so,
      * match_foreign_keys_to_quals() will get rid of them.
      */
+
+    /*
+     * Finally, remove the rel from the baserel array to prevent it from being
+     * referenced again.  (We can't do this earlier because
+     * remove_join_clause_from_rels will touch it.)
+     */
+    root->simple_rel_array[relid] = NULL;
+
+    /* And nuke the RelOptInfo, just in case there's another access path */
+    pfree(rel);
 }

 /*
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 0d4b1ec4e4..278f7ae80e 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -823,8 +823,7 @@ typedef enum RelOptKind
     RELOPT_OTHER_MEMBER_REL,
     RELOPT_OTHER_JOINREL,
     RELOPT_UPPER_REL,
-    RELOPT_OTHER_UPPER_REL,
-    RELOPT_DEADREL
+    RELOPT_OTHER_UPPER_REL
 } RelOptKind;

 /*