From 6fd5c5e265099ca0167cdb6db401bfbb08d38c2b Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Fri, 27 Aug 2021 10:47:00 +0200 Subject: [PATCH] Fix record hash support, variant 1 --- src/backend/parser/analyze.c | 23 ++++++++++++++++++----- src/backend/rewrite/rewriteSearchCycle.c | 6 +++--- src/backend/utils/adt/arrayfuncs.c | 22 ++++++++++++++++++++-- src/backend/utils/cache/typcache.c | 11 +++++++---- src/include/parser/analyze.h | 2 +- 5 files changed, 49 insertions(+), 15 deletions(-) diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index 15669c8238..6af6f09f94 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -1852,9 +1852,12 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt) /* * Make a SortGroupClause node for a SetOperationStmt's groupClauses + * + * If need_hash is true, the caller is indicating that they need hash support + * or they will fail. So look extra hard for hash support. */ SortGroupClause * -makeSortGroupClauseForSetOp(Oid rescoltype) +makeSortGroupClauseForSetOp(Oid rescoltype, bool need_hash) { SortGroupClause *grpcl = makeNode(SortGroupClause); Oid sortop; @@ -1867,6 +1870,15 @@ makeSortGroupClauseForSetOp(Oid rescoltype) &sortop, &eqop, NULL, &hashable); + /* + * The type cache doesn't believe that record is hashable (see + * cache_record_field_properties()), but if the caller really needs hash + * support, we can assume it does. Worst case, if any components of the + * record don't support hashing, we will fail at execution. + */ + if (need_hash && (rescoltype == RECORDOID || rescoltype == RECORDARRAYOID)) + hashable = true; + /* we don't have a tlist yet, so can't assign sortgrouprefs */ grpcl->tleSortGroupRef = 0; grpcl->eqop = eqop; @@ -2027,6 +2039,8 @@ transformSetOperationTree(ParseState *pstate, SelectStmt *stmt, ListCell *ltl; ListCell *rtl; const char *context; + bool recursive = (pstate->p_parent_cte && + pstate->p_parent_cte->cterecursive); context = (stmt->op == SETOP_UNION ? "UNION" : (stmt->op == SETOP_INTERSECT ? "INTERSECT" : @@ -2048,9 +2062,7 @@ transformSetOperationTree(ParseState *pstate, SelectStmt *stmt, * containing CTE as having those result columns. We should do this * only at the topmost setop of the CTE, of course. */ - if (isTopLevel && - pstate->p_parent_cte && - pstate->p_parent_cte->cterecursive) + if (isTopLevel && recursive) determineRecursiveColTypes(pstate, op->larg, ltargetlist); /* @@ -2182,8 +2194,9 @@ transformSetOperationTree(ParseState *pstate, SelectStmt *stmt, setup_parser_errposition_callback(&pcbstate, pstate, bestlocation); + /* If it's a recursive union, we need to require hashing support. */ op->groupClauses = lappend(op->groupClauses, - makeSortGroupClauseForSetOp(rescoltype)); + makeSortGroupClauseForSetOp(rescoltype, recursive)); cancel_parser_errposition_callback(&pcbstate); } diff --git a/src/backend/rewrite/rewriteSearchCycle.c b/src/backend/rewrite/rewriteSearchCycle.c index c50ebdba24..ef38f4025a 100644 --- a/src/backend/rewrite/rewriteSearchCycle.c +++ b/src/backend/rewrite/rewriteSearchCycle.c @@ -594,7 +594,7 @@ rewriteSearchAndCycle(CommonTableExpr *cte) sos->colCollations = lappend_oid(sos->colCollations, InvalidOid); if (!sos->all) sos->groupClauses = lappend(sos->groupClauses, - makeSortGroupClauseForSetOp(search_seq_type)); + makeSortGroupClauseForSetOp(search_seq_type, true)); } if (cte->cycle_clause) { @@ -603,14 +603,14 @@ rewriteSearchAndCycle(CommonTableExpr *cte) sos->colCollations = lappend_oid(sos->colCollations, cte->cycle_clause->cycle_mark_collation); if (!sos->all) sos->groupClauses = lappend(sos->groupClauses, - makeSortGroupClauseForSetOp(cte->cycle_clause->cycle_mark_type)); + makeSortGroupClauseForSetOp(cte->cycle_clause->cycle_mark_type, true)); sos->colTypes = lappend_oid(sos->colTypes, RECORDARRAYOID); sos->colTypmods = lappend_int(sos->colTypmods, -1); sos->colCollations = lappend_oid(sos->colCollations, InvalidOid); if (!sos->all) sos->groupClauses = lappend(sos->groupClauses, - makeSortGroupClauseForSetOp(RECORDARRAYOID)); + makeSortGroupClauseForSetOp(RECORDARRAYOID, true)); } /* diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c index 04d487c544..e7209b4d6b 100644 --- a/src/backend/utils/adt/arrayfuncs.c +++ b/src/backend/utils/adt/arrayfuncs.c @@ -29,6 +29,7 @@ #include "utils/arrayaccess.h" #include "utils/builtins.h" #include "utils/datum.h" +#include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/selfuncs.h" @@ -3960,6 +3961,8 @@ hash_array(PG_FUNCTION_ARGS) char typalign; int i; array_iter iter; + FmgrInfo record_hash_proc_finfo; + FmgrInfo *my_hash_proc_finfo; /* * We arrange to look up the hash function only once per series of calls, @@ -3973,7 +3976,7 @@ hash_array(PG_FUNCTION_ARGS) { typentry = lookup_type_cache(element_type, TYPECACHE_HASH_PROC_FINFO); - if (!OidIsValid(typentry->hash_proc_finfo.fn_oid)) + if (!OidIsValid(typentry->hash_proc_finfo.fn_oid) && element_type != RECORDOID) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_FUNCTION), errmsg("could not identify a hash function for type %s", @@ -3984,10 +3987,25 @@ hash_array(PG_FUNCTION_ARGS) typbyval = typentry->typbyval; typalign = typentry->typalign; + /* + * The type cache doesn't believe that record is hashable (see + * cache_record_field_properties()), but since we're here, we're committed + * to hashing, so we can assume it does. Worst case, if any components of + * the record don't support hashing, we will fail at execution. + */ + if (element_type == RECORDOID) + { + fmgr_info_cxt(F_HASH_RECORD, &record_hash_proc_finfo, + CacheMemoryContext); + my_hash_proc_finfo = &record_hash_proc_finfo; + } + else + my_hash_proc_finfo = &typentry->hash_proc_finfo; + /* * apply the hash function to each array element. */ - InitFunctionCallInfoData(*locfcinfo, &typentry->hash_proc_finfo, 1, + InitFunctionCallInfoData(*locfcinfo, my_hash_proc_finfo, 1, PG_GET_COLLATION(), NULL, NULL); /* Loop over source data */ diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 326fae62e2..70e5c51297 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -1515,14 +1515,17 @@ cache_record_field_properties(TypeCacheEntry *typentry) /* * For type RECORD, we can't really tell what will work, since we don't * have access here to the specific anonymous type. Just assume that - * everything will (we may get a failure at runtime ...) + * equality and comparison will (we may get a failure at runtime). We + * could also claim that hashing works, but then if code that has the + * option between a comparison-based (sort-based) and a hash-based plan + * chooses hashing, stuff could fail that would otherwise work if it chose + * a comparison-based plan. In practice more types support comparison + * than hashing. */ if (typentry->type_id == RECORDOID) { typentry->flags |= (TCFLAGS_HAVE_FIELD_EQUALITY | - TCFLAGS_HAVE_FIELD_COMPARE | - TCFLAGS_HAVE_FIELD_HASHING | - TCFLAGS_HAVE_FIELD_EXTENDED_HASHING); + TCFLAGS_HAVE_FIELD_COMPARE); } else if (typentry->typtype == TYPTYPE_COMPOSITE) { diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.h index 6716db6c13..89d61238e9 100644 --- a/src/include/parser/analyze.h +++ b/src/include/parser/analyze.h @@ -48,6 +48,6 @@ extern void applyLockingClause(Query *qry, Index rtindex, extern List *BuildOnConflictExcludedTargetlist(Relation targetrel, Index exclRelIndex); -extern SortGroupClause *makeSortGroupClauseForSetOp(Oid rescoltype); +extern SortGroupClause *makeSortGroupClauseForSetOp(Oid rescoltype, bool need_hash); #endif /* ANALYZE_H */ -- 2.33.0