From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David Rowley <dgrowleyml@gmail.com>
Cc: sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #17158: Distinct ROW fails with Postgres 14
Date: Fri, 27 Aug 2021 12:36:04 +0200
Message-ID: <c182f5c8-fdf3-80a0-fa43-4ed7e87d4d47@enterprisedb.com> (raw)
In-Reply-To: <496046.1629843371@sss.pgh.pa.us>
References: <17158-8a2ba823982537a4@postgresql.org>
<CAApHDvp0w_5YY=7pxPkymtv7Oc0cGS=+jtiBvXrfcM6gLjT5Pg@mail.gmail.com>
<bc0ec85a-68cf-1b99-e80c-ee0c55b936db@enterprisedb.com>
<496046.1629843371@sss.pgh.pa.us>
On 25.08.21 00:16, Tom Lane wrote:
> Undoing that would lose v14's ability to select hashed duplicate
> elimination for RECORD columns, but that's still not a regression
> because we didn't have it before. Moreover, anyone who's unhappy can
> work around the problem by explicitly casting the column to some
> suitable named composite type. We can leave it for later to make the
> planner smarter about anonymous record types. It clearly could be
> smarter, at least for the case of an explicit ROW construct at top
> level; but now is no time to be writing such code for v14.
This feature is a requirement for multicolumn path and cycle tracking in
recursive queries, as well as the search/cycle syntax built on top of
that, so there is a bit more depending on it than might be at first
apparent.
I've been looking at ways to repair this with minimal impact.
Essentially, we'd need a way ask the type cache to distinguish between
"do you have hash support if it's guaranteed to work" versus "hash
support is my only hope, so give it to me even if you're not completely
sure it will work". Putting this directly into the type cache does not
seem feasible with the current structure. But there aren't that many
callers of TYPECACHE_HASH_PROC*, so I looked at handling it there.
Variant 1 is that we let the type cache *not* report hash support for
the record type, and let callers fill it in. In the attached patch I've
only done this for hash_array(), because that's what's needed to get the
tests to pass, but similar code would be possible for row types, range
types, etc.
Variant 2 is that we let the type cache report hash support for the
record type, like now, and then let callers override it if they have
other options. This is the second attached patch.
It's basically fifty-fifty in terms of how many places you need to touch
in either case.
With both patches, you'll see the "union" regression test fail, which
includes a test case that is equivalent to the one from this bug report
(but using money instead of bit), but the "with" test still passes,
which covers the feature I mentioned at the beginning.
Thoughts?
From 6fd5c5e265099ca0167cdb6db401bfbb08d38c2b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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
From 512ba0bdc53aa140027a18449426bf1327ba3efe Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 27 Aug 2021 10:50:49 +0200
Subject: [PATCH] Fix record hash support, variant 2
---
src/backend/optimizer/path/joinpath.c | 2 +-
src/backend/optimizer/plan/initsplan.c | 2 +-
src/backend/parser/analyze.c | 18 +++++++++++++-----
src/backend/parser/parse_clause.c | 6 ++++++
src/backend/rewrite/rewriteSearchCycle.c | 6 +++---
src/backend/utils/cache/lsyscache.c | 2 +-
src/include/parser/analyze.h | 2 +-
7 files changed, 26 insertions(+), 12 deletions(-)
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.cindex 6407ede12a..385a41606e 100644--- a/src/backend/optimizer/path/joinpath.c+++ b/src/backend/optimizer/path/joinpath.c@@ -437,7 +437,7 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
TYPECACHE_HASH_PROC | TYPECACHE_EQ_OPR);
/* can't use a memoize node without a valid hash equals operator */
- if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr))+ if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr) || typentry->type_id == RECORDOID)
{
list_free(*operators);
list_free(*param_exprs);
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.cindex e25dc9a7ca..5fc411dcce 100644--- a/src/backend/optimizer/plan/initsplan.c+++ b/src/backend/optimizer/plan/initsplan.c@@ -2733,7 +2733,7 @@ check_memoizable(RestrictInfo *restrictinfo)
typentry = lookup_type_cache(exprType(leftarg), TYPECACHE_HASH_PROC |
TYPECACHE_EQ_OPR);
- if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr))+ if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr) || typentry->type_id == RECORDOID)
return;
restrictinfo->hasheqoperator = typentry->eq_opr;
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.cindex 15669c8238..07623f1daa 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,9 @@ makeSortGroupClauseForSetOp(Oid rescoltype)
&sortop, &eqop, NULL,
&hashable);
+ if (rescoltype == RECORDOID)+ hashable = need_hash;+
/* we don't have a tlist yet, so can't assign sortgrouprefs */
grpcl->tleSortGroupRef = 0;
grpcl->eqop = eqop;
@@ -2027,6 +2033,9 @@ 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 +2057,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 +2189,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/parser/parse_clause.c b/src/backend/parser/parse_clause.cindex b3f151d33b..3518d359aa 100644--- a/src/backend/parser/parse_clause.c+++ b/src/backend/parser/parse_clause.c@@ -3318,6 +3318,8 @@ addTargetToSortList(ParseState *pstate, TargetEntry *tle,
true, true, false,
&sortop, &eqop, NULL,
&hashable);
+ if (restype == RECORDOID)+ hashable = false;
reverse = false;
break;
case SORTBY_DESC:
@@ -3325,6 +3327,8 @@ addTargetToSortList(ParseState *pstate, TargetEntry *tle,
false, true, true,
NULL, &eqop, &sortop,
&hashable);
+ if (restype == RECORDOID)+ hashable = false;
reverse = true;
break;
case SORTBY_USING:
@@ -3449,6 +3453,8 @@ addTargetToGroupList(ParseState *pstate, TargetEntry *tle,
false, true, false,
&sortop, &eqop, NULL,
&hashable);
+ if (restype == RECORDOID)+ hashable = false;
cancel_parser_errposition_callback(&pcbstate);
diff --git a/src/backend/rewrite/rewriteSearchCycle.c b/src/backend/rewrite/rewriteSearchCycle.cindex 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/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.cindex 4ebaa552a2..f66b09d940 100644--- a/src/backend/utils/cache/lsyscache.c+++ b/src/backend/utils/cache/lsyscache.c@@ -1421,7 +1421,7 @@ op_hashjoinable(Oid opno, Oid inputtype)
else if (opno == RECORD_EQ_OP)
{
typentry = lookup_type_cache(inputtype, TYPECACHE_HASH_PROC);
- if (typentry->hash_proc == F_HASH_RECORD)+ if (typentry->hash_proc == F_HASH_RECORD && inputtype != RECORDOID)
result = true;
}
else
diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.hindex 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
Attachments:0001-Fix-record-hash-support-variant-1.patch (text/plain, 8.1K) download
From 6fd5c5e265099ca0167cdb6db401bfbb08d38c2b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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
0001-Fix-record-hash-support-variant-2.patch (text/plain, 7.5K) download
From 512ba0bdc53aa140027a18449426bf1327ba3efe Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 27 Aug 2021 10:50:49 +0200
Subject: [PATCH] Fix record hash support, variant 2
---
src/backend/optimizer/path/joinpath.c | 2 +-
src/backend/optimizer/plan/initsplan.c | 2 +-
src/backend/parser/analyze.c | 18 +++++++++++++-----
src/backend/parser/parse_clause.c | 6 ++++++
src/backend/rewrite/rewriteSearchCycle.c | 6 +++---
src/backend/utils/cache/lsyscache.c | 2 +-
src/include/parser/analyze.h | 2 +-
7 files changed, 26 insertions(+), 12 deletions(-)
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.cindex 6407ede12a..385a41606e 100644--- a/src/backend/optimizer/path/joinpath.c+++ b/src/backend/optimizer/path/joinpath.c@@ -437,7 +437,7 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info,
TYPECACHE_HASH_PROC | TYPECACHE_EQ_OPR);
/* can't use a memoize node without a valid hash equals operator */
- if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr))+ if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr) || typentry->type_id == RECORDOID)
{
list_free(*operators);
list_free(*param_exprs);
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.cindex e25dc9a7ca..5fc411dcce 100644--- a/src/backend/optimizer/plan/initsplan.c+++ b/src/backend/optimizer/plan/initsplan.c@@ -2733,7 +2733,7 @@ check_memoizable(RestrictInfo *restrictinfo)
typentry = lookup_type_cache(exprType(leftarg), TYPECACHE_HASH_PROC |
TYPECACHE_EQ_OPR);
- if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr))+ if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr) || typentry->type_id == RECORDOID)
return;
restrictinfo->hasheqoperator = typentry->eq_opr;
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.cindex 15669c8238..07623f1daa 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,9 @@ makeSortGroupClauseForSetOp(Oid rescoltype)
&sortop, &eqop, NULL,
&hashable);
+ if (rescoltype == RECORDOID)+ hashable = need_hash;+
/* we don't have a tlist yet, so can't assign sortgrouprefs */
grpcl->tleSortGroupRef = 0;
grpcl->eqop = eqop;
@@ -2027,6 +2033,9 @@ 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 +2057,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 +2189,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/parser/parse_clause.c b/src/backend/parser/parse_clause.cindex b3f151d33b..3518d359aa 100644--- a/src/backend/parser/parse_clause.c+++ b/src/backend/parser/parse_clause.c@@ -3318,6 +3318,8 @@ addTargetToSortList(ParseState *pstate, TargetEntry *tle,
true, true, false,
&sortop, &eqop, NULL,
&hashable);
+ if (restype == RECORDOID)+ hashable = false;
reverse = false;
break;
case SORTBY_DESC:
@@ -3325,6 +3327,8 @@ addTargetToSortList(ParseState *pstate, TargetEntry *tle,
false, true, true,
NULL, &eqop, &sortop,
&hashable);
+ if (restype == RECORDOID)+ hashable = false;
reverse = true;
break;
case SORTBY_USING:
@@ -3449,6 +3453,8 @@ addTargetToGroupList(ParseState *pstate, TargetEntry *tle,
false, true, false,
&sortop, &eqop, NULL,
&hashable);
+ if (restype == RECORDOID)+ hashable = false;
cancel_parser_errposition_callback(&pcbstate);
diff --git a/src/backend/rewrite/rewriteSearchCycle.c b/src/backend/rewrite/rewriteSearchCycle.cindex 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/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.cindex 4ebaa552a2..f66b09d940 100644--- a/src/backend/utils/cache/lsyscache.c+++ b/src/backend/utils/cache/lsyscache.c@@ -1421,7 +1421,7 @@ op_hashjoinable(Oid opno, Oid inputtype)
else if (opno == RECORD_EQ_OP)
{
typentry = lookup_type_cache(inputtype, TYPECACHE_HASH_PROC);
- if (typentry->hash_proc == F_HASH_RECORD)+ if (typentry->hash_proc == F_HASH_RECORD && inputtype != RECORDOID)
result = true;
}
else
diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.hindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: peter.eisentraut@enterprisedb.com, tgl@sss.pgh.pa.us, dgrowleyml@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17158: Distinct ROW fails with Postgres 14
In-Reply-To: <c182f5c8-fdf3-80a0-fa43-4ed7e87d4d47@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox