Skip to content

Commit bcddad2

Browse files
committed
Ensure mark_dummy_rel doesn't create dangling pointers in RelOptInfos.
When we are doing GEQO join planning, the current memory context is a short-lived context that will be reset at the end of geqo_eval(). However, the RelOptInfos for base relations are set up before that and then re-used across many GEQO cycles. Hence, any code that modifies a baserel during join planning has to be careful not to put pointers to the short-lived context into the baserel struct. mark_dummy_rel got this wrong, leading to easy-to-reproduce-once-you-know-how crashes in 8.4, as reported off-list by Leo Carson of SDSC. Some improvements made in 9.0 make it difficult to demonstrate the crash in 9.0 or HEAD; but there's no doubt that there's still a risk factor here, so patch all branches that have the function. (Note: 8.3 has a similar function, but it's only applied to joinrels and thus is not a hazard.)
1 parent 078a930 commit bcddad2

File tree

1 file changed

+25
-1
lines changed

1 file changed

+25
-1
lines changed

src/backend/optimizer/path/joinrels.c

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "optimizer/joininfo.h"
1818
#include "optimizer/pathnode.h"
1919
#include "optimizer/paths.h"
20+
#include "utils/memutils.h"
2021

2122

2223
static void make_rels_by_clause_joins(PlannerInfo *root,
@@ -933,11 +934,32 @@ is_dummy_rel(RelOptInfo *rel)
933934
}
934935

935936
/*
936-
* Mark a rel as proven empty.
937+
* Mark a relation as proven empty.
938+
*
939+
* During GEQO planning, this can get invoked more than once on the same
940+
* baserel struct, so it's worth checking to see if the rel is already marked
941+
* dummy.
942+
*
943+
* Also, when called during GEQO join planning, we are in a short-lived
944+
* memory context. We must make sure that the dummy path attached to a
945+
* baserel survives the GEQO cycle, else the baserel is trashed for future
946+
* GEQO cycles. On the other hand, when we are marking a joinrel during GEQO,
947+
* we don't want the dummy path to clutter the main planning context. Upshot
948+
* is that the best solution is to explicitly make the dummy path in the same
949+
* context the given RelOptInfo is in.
937950
*/
938951
static void
939952
mark_dummy_rel(RelOptInfo *rel)
940953
{
954+
MemoryContext oldcontext;
955+
956+
/* Already marked? */
957+
if (is_dummy_rel(rel))
958+
return;
959+
960+
/* No, so choose correct context to make the dummy path in */
961+
oldcontext = MemoryContextSwitchTo(GetMemoryChunkContext(rel));
962+
941963
/* Set dummy size estimate */
942964
rel->rows = 0;
943965

@@ -949,6 +971,8 @@ mark_dummy_rel(RelOptInfo *rel)
949971

950972
/* Set or update cheapest_total_path */
951973
set_cheapest(rel);
974+
975+
MemoryContextSwitchTo(oldcontext);
952976
}
953977

954978

0 commit comments

Comments
 (0)