From 39a3105678a247bbfdc132cd95db5b515b8cd7f6 Mon Sep 17 00:00:00 2001 From: David Rowley Date: Mon, 8 Nov 2021 14:40:33 +1300 Subject: [PATCH] Fix incorrect hash equality operator bug in Memoize In v14, because we don't have a field in RestrictInfo to cache both the left and right type's hash equality operator, we just restrict the scope of Memoize to only when the left and right types of a RestrictInfo are the same. In master we add another field to RestrictInfo and cache both hash equality operators. Reported-by: Jaime Casanova Author: David Rowley Discussion: https://postgr.es/m/20210929185544.GB24346%40ahch-to Backpatch-through: 14 --- src/backend/nodes/copyfuncs.c | 3 ++- src/backend/nodes/outfuncs.c | 3 ++- src/backend/optimizer/path/joinpath.c | 31 +++++++++++++++++------ src/backend/optimizer/plan/initsplan.c | 28 ++++++++++++++------ src/backend/optimizer/util/restrictinfo.c | 6 +++-- src/include/nodes/pathnodes.h | 5 ++-- 6 files changed, 54 insertions(+), 22 deletions(-) diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 82464c98896..ad1ea2ff2f8 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -2362,7 +2362,8 @@ _copyRestrictInfo(const RestrictInfo *from) COPY_SCALAR_FIELD(right_bucketsize); COPY_SCALAR_FIELD(left_mcvfreq); COPY_SCALAR_FIELD(right_mcvfreq); - COPY_SCALAR_FIELD(hasheqoperator); + COPY_SCALAR_FIELD(left_hasheqoperator); + COPY_SCALAR_FIELD(right_hasheqoperator); return newnode; } diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 2e5ed77e189..23f23f11dc7 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -2565,7 +2565,8 @@ _outRestrictInfo(StringInfo str, const RestrictInfo *node) WRITE_NODE_FIELD(right_em); WRITE_BOOL_FIELD(outer_is_left); WRITE_OID_FIELD(hashjoinoperator); - WRITE_OID_FIELD(hasheqoperator); + WRITE_OID_FIELD(left_hasheqoperator); + WRITE_OID_FIELD(right_hasheqoperator); } static void diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index 6407ede12a6..0f3ad8aa658 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -394,9 +394,15 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info, RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc); OpExpr *opexpr; Node *expr; + Oid hasheqoperator; - /* can't use a memoize node without a valid hash equals operator */ - if (!OidIsValid(rinfo->hasheqoperator) || + opexpr = (OpExpr *) rinfo->clause; + + /* + * Bail if the rinfo is not compatible. We need a join OpExpr + * with 2 args. + */ + if (!IsA(opexpr, OpExpr) || list_length(opexpr->args) != 2 || !clause_sides_match_join(rinfo, outerrel, innerrel)) { list_free(*operators); @@ -404,17 +410,26 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info, return false; } - /* - * We already checked that this is an OpExpr with 2 args when - * setting hasheqoperator. - */ - opexpr = (OpExpr *) rinfo->clause; if (rinfo->outer_is_left) + { expr = (Node *) linitial(opexpr->args); + hasheqoperator = rinfo->left_hasheqoperator; + } else + { expr = (Node *) lsecond(opexpr->args); + hasheqoperator = rinfo->right_hasheqoperator; + } - *operators = lappend_oid(*operators, rinfo->hasheqoperator); + /* can't do memoize if we can't hash the outer type */ + if (!OidIsValid(hasheqoperator)) + { + list_free(*operators); + list_free(*param_exprs); + return false; + } + + *operators = lappend_oid(*operators, hasheqoperator); *param_exprs = lappend(*param_exprs, expr); } } diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index e25dc9a7ca7..f6a202d900f 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -2711,15 +2711,16 @@ check_hashjoinable(RestrictInfo *restrictinfo) /* * check_memoizable * If the restrictinfo's clause is suitable to be used for a Memoize node, - * set the hasheqoperator to the hash equality operator that will be needed - * during caching. + * set the lefthasheqoperator and righthasheqoperator to the hash equality + * operator that will be needed during caching. */ static void check_memoizable(RestrictInfo *restrictinfo) { TypeCacheEntry *typentry; Expr *clause = restrictinfo->clause; - Node *leftarg; + Oid lefttype; + Oid righttype; if (restrictinfo->pseudoconstant) return; @@ -2728,13 +2729,24 @@ check_memoizable(RestrictInfo *restrictinfo) if (list_length(((OpExpr *) clause)->args) != 2) return; - leftarg = linitial(((OpExpr *) clause)->args); + lefttype = exprType(linitial(((OpExpr *) clause)->args)); - typentry = lookup_type_cache(exprType(leftarg), TYPECACHE_HASH_PROC | + typentry = lookup_type_cache(lefttype, TYPECACHE_HASH_PROC | TYPECACHE_EQ_OPR); - if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr)) - return; + if (OidIsValid(typentry->hash_proc) && OidIsValid(typentry->eq_opr)) + restrictinfo->left_hasheqoperator = typentry->eq_opr; - restrictinfo->hasheqoperator = typentry->eq_opr; + righttype = exprType(lsecond(((OpExpr *) clause)->args)); + + /* + * Lookup the right type, unless it's the same as the left type, in which + * case typentry is already pointing to the required TypeCacheEntry. + */ + if (lefttype != righttype) + typentry = lookup_type_cache(righttype, TYPECACHE_HASH_PROC | + TYPECACHE_EQ_OPR); + + if (OidIsValid(typentry->hash_proc) && OidIsValid(typentry->eq_opr)) + restrictinfo->right_hasheqoperator = typentry->eq_opr; } diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c index aa9fb3a9fa5..ebfcf918267 100644 --- a/src/backend/optimizer/util/restrictinfo.c +++ b/src/backend/optimizer/util/restrictinfo.c @@ -217,7 +217,8 @@ make_restrictinfo_internal(PlannerInfo *root, restrictinfo->left_mcvfreq = -1; restrictinfo->right_mcvfreq = -1; - restrictinfo->hasheqoperator = InvalidOid; + restrictinfo->left_hasheqoperator = InvalidOid; + restrictinfo->right_hasheqoperator = InvalidOid; return restrictinfo; } @@ -368,7 +369,8 @@ commute_restrictinfo(RestrictInfo *rinfo, Oid comm_op) result->right_bucketsize = rinfo->left_bucketsize; result->left_mcvfreq = rinfo->right_mcvfreq; result->right_mcvfreq = rinfo->left_mcvfreq; - result->hasheqoperator = InvalidOid; + result->left_hasheqoperator = InvalidOid; + result->right_hasheqoperator = InvalidOid; return result; } diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 2a53a6e344b..186e89905b2 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -2122,8 +2122,9 @@ typedef struct RestrictInfo Selectivity left_mcvfreq; /* left side's most common val's freq */ Selectivity right_mcvfreq; /* right side's most common val's freq */ - /* hash equality operator used for memoize nodes, else InvalidOid */ - Oid hasheqoperator; + /* hash equality operators used for memoize nodes, else InvalidOid */ + Oid left_hasheqoperator; + Oid right_hasheqoperator; } RestrictInfo; /*