Allow Memoize to operate in binary comparison mode

Memoize would always use the hash equality operator for the cache key
types to determine if the current set of parameters were the same as some
previously cached set.  Certain types such as floating points where -0.0
and +0.0 differ in their binary representation but are classed as equal by
the hash equality operator may cause problems as unless the join uses the
same operator it's possible that whichever join operator is being used
would be able to distinguish the two values.  In which case we may
accidentally return in the incorrect rows out of the cache.

To fix this here we add a binary mode to Memoize to allow it to the
current set of parameters to previously cached values by comparing
bit-by-bit rather than logically using the hash equality operator.  This
binary mode is always used for LATERAL joins and it's used for normal
joins when any of the join operators are not hashable.

Reported-by: Tom Lane
Author: David Rowley
Discussion: https://postgr.es/m/3004308.1632952496@sss.pgh.pa.us
Backpatch-through: 14, where Memoize was added

Author: david-rowley

contrib/postgres_fdw/expected/postgres_fdw.out

@@ -2247,6 +2247,7 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM
                Output: t1."C 1", t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
          ->  Memoize
                Cache Key: t1.c2
+               Cache Mode: binary
                ->  Subquery Scan on q
                      ->  HashAggregate
                            Output: t2.c1, t3.c1
@@ -2255,7 +2256,7 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM
                                  Output: t2.c1, t3.c1
                                  Relations: (public.ft1 t2) INNER JOIN (public.ft2 t3)
                                  Remote SQL: SELECT r1."C 1", r2."C 1" FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1")) AND ((r1.c2 = $1::integer))))
-(16 rows)
+(17 rows)
 
 SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM ft1 t2, ft2 t3 WHERE t2.c1 = t3.c1 AND t2.c2 = t1.c2) q ORDER BY t1."C 1" OFFSET 10 LIMIT 10;
  C 1 

src/backend/commands/explain.c

@@ -3127,11 +3127,14 @@ show_memoize_info(MemoizeState *mstate, List *ancestors, ExplainState *es)
 	if (es->format != EXPLAIN_FORMAT_TEXT)
 	{
 		ExplainPropertyText("Cache Key", keystr.data, es);
+		ExplainPropertyText("Cache Mode", mstate->binary_mode ? "binary" : "logical", es);
 	}
 	else
 	{
 		ExplainIndentText(es);
 		appendStringInfo(es->str, "Cache Key: %s\n", keystr.data);
+		ExplainIndentText(es);
+		appendStringInfo(es->str, "Cache Mode: %s\n", mstate->binary_mode ? "binary" : "logical");
 	}
 
 	pfree(keystr.data);

src/backend/executor/nodeMemoize.c

@@ -71,6 +71,7 @@
 #include "executor/nodeMemoize.h"
 #include "lib/ilist.h"
 #include "miscadmin.h"
+#include "utils/datum.h"
 #include "utils/lsyscache.h"
 
 /* States of the ExecMemoize state machine */
@@ -131,7 +132,7 @@ typedef struct MemoizeEntry
 
 static uint32 MemoizeHash_hash(struct memoize_hash *tb,
 							   const MemoizeKey *key);
-static int	MemoizeHash_equal(struct memoize_hash *tb,
+static bool MemoizeHash_equal(struct memoize_hash *tb,
 							  const MemoizeKey *params1,
 							  const MemoizeKey *params2);
 
@@ -140,7 +141,7 @@ static int	MemoizeHash_equal(struct memoize_hash *tb,
 #define SH_KEY_TYPE MemoizeKey *
 #define SH_KEY key
 #define SH_HASH_KEY(tb, key) MemoizeHash_hash(tb, key)
-#define SH_EQUAL(tb, a, b) (MemoizeHash_equal(tb, a, b) == 0)
+#define SH_EQUAL(tb, a, b) MemoizeHash_equal(tb, a, b)
 #define SH_SCOPE static inline
 #define SH_STORE_HASH
 #define SH_GET_HASH(tb, a) a->hash
@@ -160,21 +161,45 @@ MemoizeHash_hash(struct memoize_hash *tb, const MemoizeKey *key)
 	TupleTableSlot *pslot = mstate->probeslot;
 	uint32		hashkey = 0;
 	int			numkeys = mstate->nkeys;
-	FmgrInfo   *hashfunctions = mstate->hashfunctions;
-	Oid		   *collations = mstate->collations;
 
-	for (int i = 0; i < numkeys; i++)
+	if (mstate->binary_mode)
 	{
-		/* rotate hashkey left 1 bit at each step */
-		hashkey = (hashkey << 1) | ((hashkey & 0x80000000) ? 1 : 0);
+		for (int i = 0; i < numkeys; i++)
+		{
+			/* rotate hashkey left 1 bit at each step */
+			hashkey = (hashkey << 1) | ((hashkey & 0x80000000) ? 1 : 0);
+
+			if (!pslot->tts_isnull[i])	/* treat nulls as having hash key 0 */
+			{
+				FormData_pg_attribute *attr;
+				uint32		hkey;
+
+				attr = &pslot->tts_tupleDescriptor->attrs[i];
+
+				hkey = datum_image_hash(pslot->tts_values[i], attr->attbyval, attr->attlen);
+
+				hashkey ^= hkey;
+			}
+		}
+	}
+	else
+	{
+		FmgrInfo   *hashfunctions = mstate->hashfunctions;
+		Oid		   *collations = mstate->collations;
 
-		if (!pslot->tts_isnull[i])	/* treat nulls as having hash key 0 */
+		for (int i = 0; i < numkeys; i++)
 		{
-			uint32		hkey;
+			/* rotate hashkey left 1 bit at each step */
+			hashkey = (hashkey << 1) | ((hashkey & 0x80000000) ? 1 : 0);
+
+			if (!pslot->tts_isnull[i])	/* treat nulls as having hash key 0 */
+			{
+				uint32		hkey;
 
-			hkey = DatumGetUInt32(FunctionCall1Coll(&hashfunctions[i],
-													collations[i], pslot->tts_values[i]));
-			hashkey ^= hkey;
+				hkey = DatumGetUInt32(FunctionCall1Coll(&hashfunctions[i],
+														collations[i], pslot->tts_values[i]));
+				hashkey ^= hkey;
+			}
 		}
 	}
 
@@ -187,7 +212,7 @@ MemoizeHash_hash(struct memoize_hash *tb, const MemoizeKey *key)
  *		table lookup.  'key2' is never used.  Instead the MemoizeState's
  *		probeslot is always populated with details of what's being looked up.
  */
-static int
+static bool
 MemoizeHash_equal(struct memoize_hash *tb, const MemoizeKey *key1,
 				  const MemoizeKey *key2)
 {
@@ -199,9 +224,38 @@ MemoizeHash_equal(struct memoize_hash *tb, const MemoizeKey *key1,
 	/* probeslot should have already been prepared by prepare_probe_slot() */
 	ExecStoreMinimalTuple(key1->params, tslot, false);
 
-	econtext->ecxt_innertuple = tslot;
-	econtext->ecxt_outertuple = pslot;
-	return !ExecQualAndReset(mstate->cache_eq_expr, econtext);
+	if (mstate->binary_mode)
+	{
+		int			numkeys = mstate->nkeys;
+
+		slot_getallattrs(tslot);
+		slot_getallattrs(pslot);
+
+		for (int i = 0; i < numkeys; i++)
+		{
+			FormData_pg_attribute *attr;
+
+			if (tslot->tts_isnull[i] != pslot->tts_isnull[i])
+				return false;
+
+			/* both NULL? they're equal */
+			if (tslot->tts_isnull[i])
+				continue;
+
+			/* perform binary comparison on the two datums */
+			attr = &tslot->tts_tupleDescriptor->attrs[i];
+			if (!datum_image_eq(tslot->tts_values[i], pslot->tts_values[i],
+								attr->attbyval, attr->attlen))
+				return false;
+		}
+		return true;
+	}
+	else
+	{
+		econtext->ecxt_innertuple = tslot;
+		econtext->ecxt_outertuple = pslot;
+		return ExecQualAndReset(mstate->cache_eq_expr, econtext);
+	}
 }
 
 /*
@@ -926,6 +980,12 @@ ExecInitMemoize(Memoize *node, EState *estate, int eflags)
 	 */
 	mstate->singlerow = node->singlerow;
 
+	/*
+	 * Record if the cache keys should be compared bit by bit, or logically
+	 * using the type's hash equality operator
+	 */
+	mstate->binary_mode = node->binary_mode;
+
 	/* Zero the statistics counters */
 	memset(&mstate->stats, 0, sizeof(MemoizeInstrumentation));
 

src/backend/nodes/copyfuncs.c

@@ -971,6 +971,7 @@ _copyMemoize(const Memoize *from)
 	COPY_POINTER_FIELD(collations, sizeof(Oid) * from->numKeys);
 	COPY_NODE_FIELD(param_exprs);
 	COPY_SCALAR_FIELD(singlerow);
+	COPY_SCALAR_FIELD(binary_mode);
 	COPY_SCALAR_FIELD(est_entries);
 
 	return newnode;

src/backend/nodes/outfuncs.c

@@ -866,6 +866,7 @@ _outMemoize(StringInfo str, const Memoize *node)
 	WRITE_OID_ARRAY(collations, node->numKeys);
 	WRITE_NODE_FIELD(param_exprs);
 	WRITE_BOOL_FIELD(singlerow);
+	WRITE_BOOL_FIELD(binary_mode);
 	WRITE_UINT_FIELD(est_entries);
 }
 
@@ -1966,6 +1967,7 @@ _outMemoizePath(StringInfo str, const MemoizePath *node)
 	WRITE_NODE_FIELD(hash_operators);
 	WRITE_NODE_FIELD(param_exprs);
 	WRITE_BOOL_FIELD(singlerow);
+	WRITE_BOOL_FIELD(binary_mode);
 	WRITE_FLOAT_FIELD(calls, "%.0f");
 	WRITE_UINT_FIELD(est_entries);
 }

src/backend/nodes/readfuncs.c

@@ -2230,6 +2230,7 @@ _readMemoize(void)
 	READ_OID_ARRAY(collations, local_node->numKeys);
 	READ_NODE_FIELD(param_exprs);
 	READ_BOOL_FIELD(singlerow);
+	READ_BOOL_FIELD(binary_mode);
 	READ_UINT_FIELD(est_entries);
 
 	READ_DONE();

src/backend/optimizer/path/joinpath.c

@@ -371,19 +371,21 @@ allow_star_schema_join(PlannerInfo *root,
  *		Returns true the hashing is possible, otherwise return false.
  *
  * Additionally we also collect the outer exprs and the hash operators for
- * each parameter to innerrel.  These set in 'param_exprs' and 'operators'
- * when we return true.
+ * each parameter to innerrel.  These set in 'param_exprs', 'operators' and
+ * 'binary_mode' when we return true.
  */
 static bool
 paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
 							RelOptInfo *outerrel, RelOptInfo *innerrel,
-							List **param_exprs, List **operators)
+							List **param_exprs, List **operators,
+							bool *binary_mode)
 
 {
 	ListCell   *lc;
 
 	*param_exprs = NIL;
 	*operators = NIL;
+	*binary_mode = false;
 
 	if (param_info != NULL)
 	{
@@ -431,6 +433,20 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
 
 			*operators = lappend_oid(*operators, hasheqoperator);
 			*param_exprs = lappend(*param_exprs, expr);
+
+			/*
+			 * When the join operator is not hashable then it's possible that
+			 * the operator will be able to distinguish something that the
+			 * hash equality operator could not. For example with floating
+			 * point types -0.0 and +0.0 are classed as equal by the hash
+			 * function and equality function, but some other operator may be
+			 * able to tell those values apart.  This means that we must put
+			 * memoize into binary comparison mode so that it does bit-by-bit
+			 * comparisons rather than a "logical" comparison as it would
+			 * using the hash equality operator.
+			 */
+			if (!OidIsValid(rinfo->hashjoinoperator))
+				*binary_mode = true;
 		}
 	}
 
@@ -461,6 +477,17 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
 
 		*operators = lappend_oid(*operators, typentry->eq_opr);
 		*param_exprs = lappend(*param_exprs, expr);
+
+		/*
+		 * We must go into binary mode as we don't have too much of an idea of
+		 * how these lateral Vars are being used.  See comment above when we
+		 * set *binary_mode for the non-lateral Var case. This could be
+		 * relaxed a bit if we had the RestrictInfos and knew the operators
+		 * being used, however for cases like Vars that are arguments to
+		 * functions we must operate in binary mode as we don't have
+		 * visibility into what the function is doing with the Vars.
+		 */
+		*binary_mode = true;
 	}
 
 	/* We're okay to use memoize */
@@ -481,6 +508,7 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel,
 	List	   *param_exprs;
 	List	   *hash_operators;
 	ListCell   *lc;
+	bool		binary_mode;
 
 	/* Obviously not if it's disabled */
 	if (!enable_memoize)
@@ -572,14 +600,16 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel,
 									outerrel,
 									innerrel,
 									&param_exprs,
-									&hash_operators))
+									&hash_operators,
+									&binary_mode))
 	{
 		return (Path *) create_memoize_path(root,
 											innerrel,
 											inner_path,
 											param_exprs,
 											hash_operators,
 											extra->inner_unique,
+											binary_mode,
 											outer_path->parent->rows);
 	}
 

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, bool binary_mode,
+							 uint32 est_entries);
 static WindowAgg *make_windowagg(List *tlist, Index winref,
 								 int partNumCols, AttrNumber *partColIdx, Oid *partOperators, Oid *partCollations,
 								 int ordNumCols, AttrNumber *ordColIdx, Oid *ordOperators, Oid *ordCollations,
@@ -1617,7 +1618,8 @@ create_memoize_plan(PlannerInfo *root, MemoizePath *best_path, int flags)
 	}
 
 	plan = make_memoize(subplan, operators, collations, param_exprs,
-						best_path->singlerow, best_path->est_entries);
+						best_path->singlerow, best_path->binary_mode,
+						best_path->est_entries);
 
 	copy_generic_path_info(&plan->plan, (Path *) best_path);
 
@@ -6417,7 +6419,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, bool binary_mode,
+			 uint32 est_entries)
 {
 	Memoize    *node = makeNode(Memoize);
 	Plan	   *plan = &node->plan;
@@ -6432,6 +6435,7 @@ make_memoize(Plan *lefttree, Oid *hashoperators, Oid *collations,
 	node->collations = collations;
 	node->param_exprs = param_exprs;
 	node->singlerow = singlerow;
+	node->binary_mode = binary_mode;
 	node->est_entries = est_entries;
 
 	return node;

src/backend/optimizer/util/pathnode.c

@@ -1583,7 +1583,7 @@ create_material_path(RelOptInfo *rel, Path *subpath)
 MemoizePath *
 create_memoize_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath,
 					List *param_exprs, List *hash_operators,
-					bool singlerow, double calls)
+					bool singlerow, bool binary_mode, double calls)
 {
 	MemoizePath *pathnode = makeNode(MemoizePath);
 
@@ -1603,6 +1603,7 @@ create_memoize_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath,
 	pathnode->hash_operators = hash_operators;
 	pathnode->param_exprs = param_exprs;
 	pathnode->singlerow = singlerow;
+	pathnode->binary_mode = binary_mode;
 	pathnode->calls = calls;
 
 	/*
@@ -3942,6 +3943,7 @@ reparameterize_path(PlannerInfo *root, Path *path,
 													mpath->param_exprs,
 													mpath->hash_operators,
 													mpath->singlerow,
+													mpath->binary_mode,
 													mpath->calls);
 			}
 		default: