memoize_flush_cache_when_noncache_param_changes.patch

application/octet-stream

Filename: memoize_flush_cache_when_noncache_param_changes.patch
Type: application/octet-stream
Part: 0
Message: Re: BUG #17213: Wrong result from a query involving Merge Semi Join and Memoize

Patch

Format: unified
File+
src/backend/executor/nodeMemoize.c 38 0
src/backend/nodes/bitmapset.c 2 0
src/backend/optimizer/plan/createplan.c 10 3
src/backend/optimizer/util/clauses.c 31 0
src/include/nodes/execnodes.h 2 0
src/include/nodes/plannodes.h 1 0
src/include/optimizer/clauses.h 2 0
diff --git a/src/backend/executor/nodeMemoize.c b/src/backend/executor/nodeMemoize.c
index bec588b3a0..f6f2d52f70 100644
--- a/src/backend/executor/nodeMemoize.c
+++ b/src/backend/executor/nodeMemoize.c
@@ -313,6 +313,37 @@ remove_cache_entry(MemoizeState *mstate, MemoizeEntry *entry)
 	pfree(key);
 }
 
+/*
+ * cache_purge_all
+ *		Remove all items from the cache
+ */
+static void
+cache_purge_all(MemoizeState *mstate)
+{
+	uint64		evictions = mstate->hashtable->members;
+	PlanState *pstate = (PlanState *) mstate;
+
+	/*
+	 * Likely the most efficient way to remove all items is to just reset the
+	 * memory context for the cache and then rebuild a fresh hash table.  This
+	 * saves having to remove each item one by one and pfree each cached tuple
+	 */
+	MemoryContextReset(mstate->tableContext);
+
+	/* Make the hash table the same size as the original size */
+	build_hash_table(mstate, ((Memoize *) pstate->plan)->est_entries);
+
+	/* reset the LRU list */
+	dlist_init(&mstate->lru_list);
+	mstate->last_tuple = NULL;
+	mstate->entry = NULL;
+
+	mstate->mem_used = 0;
+
+	/* XXX should we add something new to track these purges? */
+	mstate->stats.cache_evictions += evictions; /* Update Stats */
+}
+
 /*
  * cache_reduce_memory
  *		Evict older and less recently used items from the cache in order to
@@ -925,6 +956,7 @@ ExecInitMemoize(Memoize *node, EState *estate, int eflags)
 	 * getting the first tuple.  This allows us to mark it as so.
 	 */
 	mstate->singlerow = node->singlerow;
+	mstate->keyparamids = node->keyparamids;
 
 	/* Zero the statistics counters */
 	memset(&mstate->stats, 0, sizeof(MemoizeInstrumentation));
@@ -1022,6 +1054,12 @@ ExecReScanMemoize(MemoizeState *node)
 	if (outerPlan->chgParam == NULL)
 		ExecReScan(outerPlan);
 
+	/*
+	 * Purge the entire cache if a parameter changed that is not part of the
+	 * cache key.
+	 */
+	if (bms_nonempty_difference(outerPlan->chgParam, node->keyparamids))
+		cache_purge_all(node);
 }
 
 /*
diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index 649478b0d4..4f7655a831 100644
--- a/src/backend/nodes/bitmapset.c
+++ b/src/backend/nodes/bitmapset.c
@@ -540,6 +540,8 @@ bms_overlap_list(const Bitmapset *a, const List *b)
 
 /*
  * bms_nonempty_difference - do sets have a nonempty difference?
+ *
+ * I.e are any members set in 'a' that are not also set in 'b'.
  */
 bool
 bms_nonempty_difference(const Bitmapset *a, const Bitmapset *b)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 3dc0176a51..4db3ac03ca 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -279,7 +279,8 @@ static Sort *make_sort_from_groupcols(List *groupcls,
 static Material *make_material(Plan *lefttree);
 static Memoize *make_memoize(Plan *lefttree, Oid *hashoperators,
 							 Oid *collations, List *param_exprs,
-							 bool singlerow, uint32 est_entries);
+							 bool singlerow, uint32 est_entries,
+							 Bitmapset *keyparamids);
 static WindowAgg *make_windowagg(List *tlist, Index winref,
 								 int partNumCols, AttrNumber *partColIdx, Oid *partOperators, Oid *partCollations,
 								 int ordNumCols, AttrNumber *ordColIdx, Oid *ordOperators, Oid *ordCollations,
@@ -1585,6 +1586,7 @@ static Memoize *
 create_memoize_plan(PlannerInfo *root, MemoizePath *best_path, int flags)
 {
 	Memoize    *plan;
+	Bitmapset  *keyparamids;
 	Plan	   *subplan;
 	Oid		   *operators;
 	Oid		   *collations;
@@ -1616,8 +1618,11 @@ create_memoize_plan(PlannerInfo *root, MemoizePath *best_path, int flags)
 		i++;
 	}
 
+	keyparamids = pull_paramids((Expr *) param_exprs);
+
 	plan = make_memoize(subplan, operators, collations, param_exprs,
-						best_path->singlerow, best_path->est_entries);
+						best_path->singlerow, best_path->est_entries,
+						keyparamids);
 
 	copy_generic_path_info(&plan->plan, (Path *) best_path);
 
@@ -6417,7 +6422,8 @@ materialize_finished_plan(Plan *subplan)
 
 static Memoize *
 make_memoize(Plan *lefttree, Oid *hashoperators, Oid *collations,
-			 List *param_exprs, bool singlerow, uint32 est_entries)
+			 List *param_exprs, bool singlerow, uint32 est_entries,
+			 Bitmapset *keyparamids)
 {
 	Memoize    *node = makeNode(Memoize);
 	Plan	   *plan = &node->plan;
@@ -6433,6 +6439,7 @@ make_memoize(Plan *lefttree, Oid *hashoperators, Oid *collations,
 	node->param_exprs = param_exprs;
 	node->singlerow = singlerow;
 	node->est_entries = est_entries;
+	node->keyparamids = keyparamids;
 
 	return node;
 }
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 3412d31117..61d9d5899a 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -152,6 +152,7 @@ static Query *substitute_actual_srf_parameters(Query *expr,
 											   int nargs, List *args);
 static Node *substitute_actual_srf_parameters_mutator(Node *node,
 													  substitute_actual_srf_parameters_context *context);
+static bool pull_paramids_walker(Node *node, Bitmapset **context);
 
 
 /*****************************************************************************
@@ -5214,3 +5215,33 @@ substitute_actual_srf_parameters_mutator(Node *node,
 								   substitute_actual_srf_parameters_mutator,
 								   (void *) context);
 }
+
+/*
+ * pull_paramids
+ *		Returns a Bitmapset containing the paramids of all Params in 'expr'.
+ */
+Bitmapset *
+pull_paramids(Expr *expr)
+{
+	Bitmapset  *result = NULL;
+
+	(void) pull_paramids_walker((Node *) expr, &result);
+
+	return result;
+}
+
+static bool
+pull_paramids_walker(Node *node, Bitmapset **context)
+{
+	if (node == NULL)
+		return false;
+	if (IsA(node, Param))
+	{
+		Param	   *param = (Param *)node;
+
+		*context = bms_add_member(*context, param->paramid);
+		return false;
+	}
+	return expression_tree_walker(node, pull_paramids_walker,
+								  (void *) context);
+}
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 2e8cbee69f..3c60743e4b 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2111,6 +2111,8 @@ typedef struct MemoizeState
 								 * complete after caching the first tuple. */
 	MemoizeInstrumentation stats;	/* execution statistics */
 	SharedMemoizeInfo *shared_info; /* statistics for parallel workers */
+	Bitmapset	   *keyparamids; /* Param->paramids of expressions belonging to
+								  * param_exprs */
 } MemoizeState;
 
 /* ----------------
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 01a246d50e..c0789ab26c 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -802,6 +802,7 @@ typedef struct Memoize
 	uint32		est_entries;	/* The maximum number of entries that the
 								 * planner expects will fit in the cache, or 0
 								 * if unknown */
+	Bitmapset   *keyparamids;	/* paramids from param_exprs */
 } Memoize;
 
 /* ----------------
diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h
index 0673887a85..bc3f3e60d4 100644
--- a/src/include/optimizer/clauses.h
+++ b/src/include/optimizer/clauses.h
@@ -53,4 +53,6 @@ extern void CommuteOpExpr(OpExpr *clause);
 extern Query *inline_set_returning_function(PlannerInfo *root,
 											RangeTblEntry *rte);
 
+extern Bitmapset *pull_paramids(Expr *expr);
+
 #endif							/* CLAUSES_H */