From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Subject: Killing off removed rels properly
Date: Fri, 10 Feb 2023 15:50:20 -0500
Message-ID: <229905.1676062220@sss.pgh.pa.us> (raw)
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
Attachments:really-remove-dead-rels-1.patch (text/x-diff, 2.1K) downloaddiff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.cindex 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.cindex 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.hindex 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;
/*
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: Killing off removed rels properly
In-Reply-To: <229905.1676062220@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox