pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17158: Distinct ROW fails with Postgres 14
14+ messages / 4 participants
[nested] [flat]
* BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-08-24 08:46 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 14+ messages in thread
From: PG Bug reporting form @ 2021-08-24 08:46 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: sait.nisanci@microsoft.com
The following bug has been logged on the website:
Bug reference: 17158
Logged by: sait talha nisanci
Email address: sait.nisanci@microsoft.com
PostgreSQL version: 14beta3
Operating system: Ubuntu 20.04
Description:
Hi,
```
Create or replace function test_jsonb() returns jsonb as
$$
begin
return '{"test_json": "test"}';
end;
$$ language plpgsql;
CREATE TABLE local
(
dist_key bigint PRIMARY KEY,
col1 int[], col2 int[][], col3 int [][][],
col4 varchar[], col5 varchar[][], col6 varchar [][][],
col70 bit, col7 bit[], col8 bit[][], col9 bit [][][],
col10 bit varying(10),
col11 bit varying(10)[], col12 bit varying(10)[][], col13 bit
varying(10)[][][],
col14 bytea, col15 bytea[], col16 bytea[][], col17 bytea[][][],
col18 boolean, col19 boolean[], col20 boolean[][], col21 boolean[][][],
col22 inet, col23 inet[], col24 inet[][], col25 inet[][][],
col26 macaddr, col27 macaddr[], col28 macaddr[][], col29 macaddr[][][],
col30 numeric, col32 numeric[], col33 numeric[][], col34 numeric[][][],
col35 jsonb, col36 jsonb[], col37 jsonb[][], col38 jsonb[][][]
);
INSERT INTO local (dist_key,col1, col2, col3, col4, col5, col6, col70, col7,
col8, col9, col10, col11, col12, col13, col14, col15, col16, col17, col18,
col19, col20, col21, col22, col23, col24, col25, col26, col27, col28, col29,
col30, col32, col33, col34, col35, col36, col37, col38)
VALUES (1,ARRAY[1], ARRAY[ARRAY[0,0,0]], ARRAY[ARRAY[ARRAY[0,0,0]]],
ARRAY['1'], ARRAY[ARRAY['0','0','0']], ARRAY[ARRAY[ARRAY['0','0','0']]],
'1', ARRAY[b'1'], ARRAY[ARRAY[b'0',b'0',b'0']],
ARRAY[ARRAY[ARRAY[b'0',b'0',b'0']]], '11101',ARRAY[b'1'],
ARRAY[ARRAY[b'01',b'01',b'01']], ARRAY[ARRAY[ARRAY[b'011',b'110',b'0000']]],
'\xb4a8e04c0b', ARRAY['\xb4a8e04c0b'::BYTEA],
ARRAY[ARRAY['\xb4a8e04c0b'::BYTEA, '\xb4a8e04c0b'::BYTEA,
'\xb4a8e04c0b'::BYTEA]],
ARRAY[ARRAY[ARRAY['\xb4a8e04c0b'::BYTEA,'\x18a232a678'::BYTEA,'\x38b2697632'::BYTEA]]],
'1', ARRAY[TRUE], ARRAY[ARRAY[1::boolean,TRUE,FALSE]],
ARRAY[ARRAY[ARRAY[1::boolean,TRUE,FALSE]]], INET '192.168.1/24', ARRAY[INET
'192.168.1.1'], ARRAY[ARRAY[INET '0.0.0.0', '0.0.0.0/32', '::ffff:fff0:1',
'192.168.1/24']], ARRAY[ARRAY[ARRAY[INET '0.0.0.0', '0.0.0.0/32',
'::ffff:fff0:1', '192.168.1/24']]],MACADDR '08:00:2b:01:02:03',
ARRAY[MACADDR '08:00:2b:01:02:03'], ARRAY[ARRAY[MACADDR '08002b-010203',
MACADDR '08002b-010203', '08002b010203']], ARRAY[ARRAY[ARRAY[MACADDR
'08002b-010203', MACADDR '08002b-010203', '08002b010203']]], 690,
ARRAY[1.1], ARRAY[ARRAY[0,0.111,0.15]], ARRAY[ARRAY[ARRAY[0,0,0]]],
test_jsonb(), ARRAY[test_jsonb()],
ARRAY[ARRAY[test_jsonb(),test_jsonb(),test_jsonb(),test_jsonb()]],
ARRAY[ARRAY[ARRAY[test_jsonb(),test_jsonb(),test_jsonb(),test_jsonb()]]]),
(2,ARRAY[1,2,3], ARRAY[ARRAY[1,2,3], ARRAY[5,6,7]],
ARRAY[ARRAY[ARRAY[1,2,3]], ARRAY[ARRAY[5,6,7]], ARRAY[ARRAY[1,2,3]],
ARRAY[ARRAY[5,6,7]]], ARRAY['1','2','3'], ARRAY[ARRAY['1','2','3'],
ARRAY['5','6','7']], ARRAY[ARRAY[ARRAY['1','2','3']],
ARRAY[ARRAY['5','6','7']], ARRAY[ARRAY['1','2','3']],
ARRAY[ARRAY['5','6','7']]], '0', ARRAY[b'1',b'0',b'0'],
ARRAY[ARRAY[b'1',b'1',b'0'], ARRAY[b'0',b'0',b'1']],
ARRAY[ARRAY[ARRAY[b'1',b'1',b'1']], ARRAY[ARRAY[b'1','0','0']],
ARRAY[ARRAY[b'1','1','1']], ARRAY[ARRAY[b'0','0','0']]], '00010',
ARRAY[b'11',b'10',b'01'], ARRAY[ARRAY[b'11',b'010',b'101'],
ARRAY[b'101',b'01111',b'1000001']],
ARRAY[ARRAY[ARRAY[b'10000',b'111111',b'1101010101']],
ARRAY[ARRAY[b'1101010','0','1']], ARRAY[ARRAY[b'1','1','11111111']],
ARRAY[ARRAY[b'0000000','0','0']]], '\xb4a8e04c0b',
ARRAY['\xb4a8e04c0b'::BYTEA,'\x18a232a678'::BYTEA,'\x38b2697632'::BYTEA],
ARRAY[ARRAY['\xb4a8e04c0b'::BYTEA,'\x18a232a678'::BYTEA,'\x38b2697632'::BYTEA],
ARRAY['\xb4a8e04c0b'::BYTEA,'\x18a232a678'::BYTEA,'\x38b2697632'::BYTEA]],
ARRAY[ARRAY[ARRAY['\xb4a8e04c0b'::BYTEA,'\x18a232a678'::BYTEA,'\x38b2697632'::BYTEA]],
ARRAY[ARRAY['\xb4a8e04c0b'::BYTEA,'\x18a232a678'::BYTEA,'\x38b2697632'::BYTEA]],
ARRAY[ARRAY['\xb4a8e04c0b'::BYTEA,'\x18a232a678'::BYTEA,'\x38b2697632'::BYTEA]],
ARRAY[ARRAY['\xb4a8e04c0b'::BYTEA,'\x18a232a678'::BYTEA,'\x38b2697632'::BYTEA]]],
'true', ARRAY[1::boolean,TRUE,FALSE], ARRAY[ARRAY[1::boolean,TRUE,FALSE],
ARRAY[1::boolean,TRUE,FALSE]], ARRAY[ARRAY[ARRAY[1::boolean,TRUE,FALSE]],
ARRAY[ARRAY[1::boolean,TRUE,FALSE]], ARRAY[ARRAY[1::boolean,TRUE,FALSE]],
ARRAY[ARRAY[1::boolean,TRUE,FALSE]]],'0.0.0.0/32', ARRAY[INET '0.0.0.0',
'0.0.0.0/32', '::ffff:fff0:1', '192.168.1/24'], ARRAY[ARRAY[INET '0.0.0.0',
'0.0.0.0/32', '::ffff:fff0:1', '192.168.1/24']], ARRAY[ARRAY[ARRAY[INET
'0.0.0.0', '0.0.0.0/32', '::ffff:fff0:1', '192.168.1/24']], ARRAY[ARRAY[INET
'0.0.0.0', '0.0.0.0/32', '::ffff:fff0:1', '192.168.1/24']], ARRAY[ARRAY[INET
'0.0.0.0', '0.0.0.0/32', '::ffff:fff0:1', '192.168.1/24']], ARRAY[ARRAY[INET
'0.0.0.0', '0.0.0.0/32', '::ffff:fff0:1', '192.168.1/24']]],
'0800.2b01.0203', ARRAY[MACADDR '08002b-010203', MACADDR '08002b-010203',
'08002b010203'], ARRAY[ARRAY[MACADDR '08002b-010203', MACADDR
'08002b-010203', '08002b010203']], ARRAY[ARRAY[ARRAY[MACADDR
'08002b-010203', MACADDR '08002b-010203', '08002b010203']],
ARRAY[ARRAY[MACADDR '08002b-010203', MACADDR '08002b-010203',
'08002b010203']], ARRAY[ARRAY[MACADDR '08002b-010203', MACADDR
'08002b-010203', '08002b010203']], ARRAY[ARRAY[MACADDR '08002b-010203',
MACADDR '08002b-010203', '08002b010203']]], 0.99, ARRAY[1.1,2.22,3.33],
ARRAY[ARRAY[1.55,2.66,3.88], ARRAY[11.5,10101.6,7111.1]],
ARRAY[ARRAY[ARRAY[1,2,3]], ARRAY[ARRAY[5,6,7]], ARRAY[ARRAY[1.1,2.1,3]],
ARRAY[ARRAY[5.0,6.0,7.0]]],test_jsonb(),
ARRAY[test_jsonb(),test_jsonb(),test_jsonb(),test_jsonb()],
ARRAY[ARRAY[test_jsonb(),test_jsonb(),test_jsonb(),test_jsonb()],
ARRAY[test_jsonb(),test_jsonb(),test_jsonb(),test_jsonb()]],
ARRAY[ARRAY[ARRAY[test_jsonb(),test_jsonb(),test_jsonb(),test_jsonb()]],
ARRAY[ARRAY[test_jsonb(),test_jsonb(),test_jsonb(),test_jsonb()]],
ARRAY[ARRAY[test_jsonb(),test_jsonb(),test_jsonb(),test_jsonb()]],
ARRAY[ARRAY[test_jsonb(),test_jsonb(),test_jsonb(),test_jsonb()]]]);
-- This works fine before pg14.
SELECT DISTINCT ROW(col1, col2, col3, col4, col5, col6, col70, col7, col8,
col9, col10,
col11, col12, col13, col14, col15, col16, col17, col18, col19, col20, col21,
col22, col23, col24, col25,
col26, col27, col28, col29, col32, col33, col34, col35, col36, col37, col38)
AS "row" FROM local WHERE true;
ERROR: could not identify a hash function for type bit
```
The `SELECT DISTINCT ROW` works fine prior to Postgres 14, so it seems like
there might be some problem with Postgres14. Is this expected?
Best,
Talha.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-08-24 09:55 David Rowley <dgrowleyml@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 2 replies; 14+ messages in thread
From: David Rowley @ 2021-08-24 09:55 UTC (permalink / raw)
To: sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>
On Tue, 24 Aug 2021 at 21:02, PG Bug reporting form
<noreply@postgresql.org> wrote:
> -- This works fine before pg14.
> SELECT DISTINCT ROW(col1, col2, col3, col4, col5, col6, col70, col7, col8,
> col9, col10,
> col11, col12, col13, col14, col15, col16, col17, col18, col19, col20, col21,
> col22, col23, col24, col25,
> col26, col27, col28, col29, col32, col33, col34, col35, col36, col37, col38)
> AS "row" FROM local WHERE true;
> ERROR: could not identify a hash function for type bit
It looks like 01e658fa74 is to blame for this.
The test case can be simplified down to just:
create table local (b bit);
insert into local values('1'),('0');
SELECT DISTINCT ROW(b) FROM local;
Tom did have a look at this and raise the question about the
possibility of not being able to hash in [1].
If it's going to be a problem detecting the lack of hashability during
planning then maybe we can just add a hash opclass for BIT to fix this
particular case.
I've copied in Peter as 01e658fa74 is one of his.
David
[1] https://www.postgresql.org/message-id/20201019233234.r6lyxbvdg5s77rvd%40alap3.anarazel.de
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-08-24 14:03 Tom Lane <tgl@sss.pgh.pa.us>
parent: David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 14+ messages in thread
From: Tom Lane @ 2021-08-24 14:03 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>
David Rowley <dgrowleyml@gmail.com> writes:
> It looks like 01e658fa74 is to blame for this.
> The test case can be simplified down to just:
> create table local (b bit);
> insert into local values('1'),('0');
> SELECT DISTINCT ROW(b) FROM local;
> Tom did have a look at this and raise the question about the
> possibility of not being able to hash in [1].
Huh. According to the thread, we discussed this exact possibility and
there's a test case verifying it ... so apparently something got
fat-fingered there.
> If it's going to be a problem detecting the lack of hashability during
> planning then maybe we can just add a hash opclass for BIT to fix this
> particular case.
Most certainly not. That would translate to a requirement that EVERY
data type have a hash function.
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-08-24 15:05 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 14+ messages in thread
From: Tom Lane @ 2021-08-24 15:05 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>
I wrote:
> David Rowley <dgrowleyml@gmail.com> writes:
>> Tom did have a look at this and raise the question about the
>> possibility of not being able to hash in [1].
> Huh. According to the thread, we discussed this exact possibility and
> there's a test case verifying it ... so apparently something got
> fat-fingered there.
I think this is on me, because I must not have looked closely enough
at Peter's test case to realize that he was proposing to consider it
okay to fail. We can't have that, for exactly the reason seen here,
which is that people will consider it a regression if their queries
used to work and now hit this error.
The proximate cause of the problem is this over-optimistic bit in
cache_record_field_properties():
/*
* 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 ...)
*/
if (typentry->type_id == RECORDOID)
{
typentry->flags |= (TCFLAGS_HAVE_FIELD_EQUALITY |
TCFLAGS_HAVE_FIELD_COMPARE |
TCFLAGS_HAVE_FIELD_HASHING |
TCFLAGS_HAVE_FIELD_EXTENDED_HASHING);
}
where 01e658fa74 just blindly added hashing to the set of things we
assume an unknown record type can do. While we seem to have mostly
gotten away with assuming that comparison ops are available, it's
clearly a step too far to assume that hashing is.
The correct long-term fix is to remove this assumption altogether
in favor of adding code to check more carefully in the planner.
But it's probably a bit late in the game to try to fix that for v14.
I propose that we just revert this code to the way it was before
(and improve the comment to explain what's going on).
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-08-24 21:25 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 14+ messages in thread
From: Peter Eisentraut @ 2021-08-24 21:25 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On 24.08.21 11:55, David Rowley wrote:
> If it's going to be a problem detecting the lack of hashability during
> planning then maybe we can just add a hash opclass for BIT to fix this
> particular case.
The following types have btree opclasses but not hash opclasses:
money
bit
bit varying
tsvector
tsquery
Also among contrib:
cube
ltree
seg
We could fix the first three relatively easily (although money is used
in test cases as not having a hash opclass). Not sure what to do about
the rest.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-08-24 22:16 Tom Lane <tgl@sss.pgh.pa.us>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2021-08-24 22:16 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 24.08.21 11:55, David Rowley wrote:
>> If it's going to be a problem detecting the lack of hashability during
>> planning then maybe we can just add a hash opclass for BIT to fix this
>> particular case.
> The following types have btree opclasses but not hash opclasses:
> money
> bit
> bit varying
> tsvector
> tsquery
> Also among contrib:
> cube
> ltree
> seg
> We could fix the first three relatively easily (although money is used
> in test cases as not having a hash opclass). Not sure what to do about
> the rest.
We can *not* institute a policy that all types must have hash opclasses,
which is what David's suggestion amounts to.
I've been thinking some more about my upthread suggestion that we just
revert cache_record_field_properties to the way it was, and I think that
it's actually pretty defensible, i.e. the lack of prior complaints isn't
all that astonishing. If a query plan involves making comparisons
(either equality or more general ordering comparisons) on a given RECORD
column, it's pretty likely that that traces directly to a semantic
requirement of the query. So the user won't/shouldn't be surprised if
he gets a failure about a component type not being able to perform the
comparison. The fact that we issue the error at run time not plan
time is a little ugly, but it'd be the same error if we had full
knowledge at plan time. On the other hand, hashing is an implementation
choice, not a semantic requirement, so users can reasonably expect the
planner to avoid using hashing when it won't work.
This argument falls down in a situation where duplicate-elimination
could be done with either hashing or sorting and the datatype has
hashing but not ordering support. I'd argue, however, that the set of
such datatypes is darn near empty. In any case, such failures are not
regressions because they never worked before either.
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.
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-08-27 10:36 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Peter Eisentraut @ 2021-08-27 10:36 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
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.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
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.c
index 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.c
index 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.c
index 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.c
index 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.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/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 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.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
Attachments:
[text/plain] 0001-Fix-record-hash-support-variant-1.patch (8.1K, ../../c182f5c8-fdf3-80a0-fa43-4ed7e87d4d47@enterprisedb.com/2-0001-Fix-record-hash-support-variant-1.patch)
download | inline diff:
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.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
[text/plain] 0001-Fix-record-hash-support-variant-2.patch (7.5K, ../../c182f5c8-fdf3-80a0-fa43-4ed7e87d4d47@enterprisedb.com/3-0001-Fix-record-hash-support-variant-2.patch)
download | inline diff:
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.c
index 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.c
index 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.c
index 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.c
index 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.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/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 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.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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-08-31 20:43 Tom Lane <tgl@sss.pgh.pa.us>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2021-08-31 20:43 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> 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.
Hmm.
> 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.
I find variant 1 a bit cleaner, and safer. I'd rather default to
assuming that RECORD doesn't hash, when we don't have enough info
to be sure.
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-09-02 08:15 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Peter Eisentraut @ 2021-09-02 08:15 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On 31.08.21 22:43, Tom Lane wrote:
> I find variant 1 a bit cleaner, and safer. I'd rather default to
> assuming that RECORD doesn't hash, when we don't have enough info
> to be sure.
Ok, here is a more polished patch for that.
From b78248ecc0fbe85e9c9091ae9180fe64c014fd97 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 2 Sep 2021 09:56:44 +0200
Subject: [PATCH v2] Disable anonymous record hash support except in special
cases
Commit 01e658fa74 added hash support for row types. This also added
support for hashing anonymous record types, using the same approach
that the type cache uses for comparison support for record types: It
just reports that it works, but it might fail at run time if a
component type doesn't actually support the operation. We get away
with that for comparison because most types support that. But some
types don't support hashing, so the current state can result in
failures at run time where the planner chooses hashing over sorting,
whereas that previously worked if only sorting was an option.
We do, however, want the record hashing support for path tracking in
recursive unions, and the SEARCH and CYCLE clauses built on that. In
that case, hashing is the only plan option. So enable that, this
commit implements the following approach: The type cache does not
report that hashing is available for the record type. This undoes
that part of 01e658fa74. Instead, callers that require hashing no
matter what can override that result themselves. This patch only
touches the callers to make the aforementioned recursive query cases
work, namely the parse analysis of unions, as well as the hash_array()
function.
Bug: #17158
---
src/backend/parser/analyze.c | 23 ++++--
src/backend/rewrite/rewriteSearchCycle.c | 6 +-
src/backend/utils/adt/arrayfuncs.c | 37 +++++++++-
src/backend/utils/cache/typcache.c | 11 +--
src/include/parser/analyze.h | 2 +-
src/test/regress/expected/union.out | 90 ++++++++++++++----------
src/test/regress/sql/union.sql | 4 +-
7 files changed, 118 insertions(+), 55 deletions(-)
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 15669c8238..146ee8dd1e 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 require_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 require_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 (require_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..557bec5889 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"
@@ -3973,13 +3974,47 @@ 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",
format_type_be(element_type))));
fcinfo->flinfo->fn_extra = (void *) typentry;
}
+
+ /*
+ * 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)
+ {
+ MemoryContext oldcontext;
+ TypeCacheEntry *record_typentry;
+
+ oldcontext = MemoryContextSwitchTo(CacheMemoryContext);
+
+ /*
+ * Make fake type cache entry structure. Note that we can't just
+ * modify typentry, since that points directly into the type cache.
+ */
+ record_typentry = palloc(sizeof(*record_typentry));
+
+ /* fill in what we need below */
+ record_typentry->typlen = typentry->typlen;
+ record_typentry->typbyval = typentry->typbyval;
+ record_typentry->typalign = typentry->typalign;
+ fmgr_info(F_HASH_RECORD, &record_typentry->hash_proc_finfo);
+
+ MemoryContextSwitchTo(oldcontext);
+
+ typentry = record_typentry;
+
+ /* also save for future calls */
+ fcinfo->flinfo->fn_extra = (void *) typentry;
+ }
+
typlen = typentry->typlen;
typbyval = typentry->typbyval;
typalign = typentry->typalign;
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..a0f0bd38d7 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 require_hash);
#endif /* ANALYZE_H */
diff --git a/src/test/regress/expected/union.out b/src/test/regress/expected/union.out
index 75f78db8f5..dece7310cf 100644
--- a/src/test/regress/expected/union.out
+++ b/src/test/regress/expected/union.out
@@ -648,34 +648,37 @@ reset enable_hashagg;
set enable_hashagg to on;
explain (costs off)
select x from (values (row(1, 2)), (row(1, 3))) _(x) union select x from (values (row(1, 2)), (row(1, 4))) _(x);
- QUERY PLAN
------------------------------------------
- HashAggregate
- Group Key: "*VALUES*".column1
- -> Append
- -> Values Scan on "*VALUES*"
- -> Values Scan on "*VALUES*_1"
-(5 rows)
+ QUERY PLAN
+-----------------------------------------------
+ Unique
+ -> Sort
+ Sort Key: "*VALUES*".column1
+ -> Append
+ -> Values Scan on "*VALUES*"
+ -> Values Scan on "*VALUES*_1"
+(6 rows)
select x from (values (row(1, 2)), (row(1, 3))) _(x) union select x from (values (row(1, 2)), (row(1, 4))) _(x);
x
-------
- (1,4)
- (1,3)
(1,2)
+ (1,3)
+ (1,4)
(3 rows)
explain (costs off)
select x from (values (row(1, 2)), (row(1, 3))) _(x) intersect select x from (values (row(1, 2)), (row(1, 4))) _(x);
- QUERY PLAN
------------------------------------------------
- HashSetOp Intersect
- -> Append
- -> Subquery Scan on "*SELECT* 1"
- -> Values Scan on "*VALUES*"
- -> Subquery Scan on "*SELECT* 2"
- -> Values Scan on "*VALUES*_1"
-(6 rows)
+ QUERY PLAN
+-----------------------------------------------------
+ SetOp Intersect
+ -> Sort
+ Sort Key: "*SELECT* 1".x
+ -> Append
+ -> Subquery Scan on "*SELECT* 1"
+ -> Values Scan on "*VALUES*"
+ -> Subquery Scan on "*SELECT* 2"
+ -> Values Scan on "*VALUES*_1"
+(8 rows)
select x from (values (row(1, 2)), (row(1, 3))) _(x) intersect select x from (values (row(1, 2)), (row(1, 4))) _(x);
x
@@ -685,15 +688,17 @@ select x from (values (row(1, 2)), (row(1, 3))) _(x) intersect select x from (va
explain (costs off)
select x from (values (row(1, 2)), (row(1, 3))) _(x) except select x from (values (row(1, 2)), (row(1, 4))) _(x);
- QUERY PLAN
------------------------------------------------
- HashSetOp Except
- -> Append
- -> Subquery Scan on "*SELECT* 1"
- -> Values Scan on "*VALUES*"
- -> Subquery Scan on "*SELECT* 2"
- -> Values Scan on "*VALUES*_1"
-(6 rows)
+ QUERY PLAN
+-----------------------------------------------------
+ SetOp Except
+ -> Sort
+ Sort Key: "*SELECT* 1".x
+ -> Append
+ -> Subquery Scan on "*SELECT* 1"
+ -> Values Scan on "*VALUES*"
+ -> Subquery Scan on "*SELECT* 2"
+ -> Values Scan on "*VALUES*_1"
+(8 rows)
select x from (values (row(1, 2)), (row(1, 3))) _(x) except select x from (values (row(1, 2)), (row(1, 4))) _(x);
x
@@ -702,21 +707,28 @@ select x from (values (row(1, 2)), (row(1, 3))) _(x) except select x from (value
(1 row)
-- non-hashable type
--- With an anonymous row type, the typcache reports that the type is
--- hashable, but then it will fail at run time.
+-- With an anonymous row type, the typcache does not report that the
+-- type is hashable. (Otherwise, this would fail at execution time.)
explain (costs off)
select x from (values (row(100::money)), (row(200::money))) _(x) union select x from (values (row(100::money)), (row(300::money))) _(x);
- QUERY PLAN
------------------------------------------
- HashAggregate
- Group Key: "*VALUES*".column1
- -> Append
- -> Values Scan on "*VALUES*"
- -> Values Scan on "*VALUES*_1"
-(5 rows)
+ QUERY PLAN
+-----------------------------------------------
+ Unique
+ -> Sort
+ Sort Key: "*VALUES*".column1
+ -> Append
+ -> Values Scan on "*VALUES*"
+ -> Values Scan on "*VALUES*_1"
+(6 rows)
select x from (values (row(100::money)), (row(200::money))) _(x) union select x from (values (row(100::money)), (row(300::money))) _(x);
-ERROR: could not identify a hash function for type money
+ x
+-----------
+ ($100.00)
+ ($200.00)
+ ($300.00)
+(3 rows)
+
-- With a defined row type, the typcache can inspect the type's fields
-- for hashability.
create type ct1 as (f1 money);
diff --git a/src/test/regress/sql/union.sql b/src/test/regress/sql/union.sql
index ce22f34c71..ca8c9b4d12 100644
--- a/src/test/regress/sql/union.sql
+++ b/src/test/regress/sql/union.sql
@@ -218,8 +218,8 @@
-- non-hashable type
--- With an anonymous row type, the typcache reports that the type is
--- hashable, but then it will fail at run time.
+-- With an anonymous row type, the typcache does not report that the
+-- type is hashable. (Otherwise, this would fail at execution time.)
explain (costs off)
select x from (values (row(100::money)), (row(200::money))) _(x) union select x from (values (row(100::money)), (row(300::money))) _(x);
select x from (values (row(100::money)), (row(200::money))) _(x) union select x from (values (row(100::money)), (row(300::money))) _(x);
base-commit: e04267844a9bbf97c2e85c919b84dfe498ab0302
--
2.33.0
Attachments:
[text/plain] v2-0001-Disable-anonymous-record-hash-support-except-in-s.patch (14.9K, ../../7fb17b3d-13dc-fc28-17ce-9c302d676bd8@enterprisedb.com/2-v2-0001-Disable-anonymous-record-hash-support-except-in-s.patch)
download | inline diff:
From b78248ecc0fbe85e9c9091ae9180fe64c014fd97 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 2 Sep 2021 09:56:44 +0200
Subject: [PATCH v2] Disable anonymous record hash support except in special
cases
Commit 01e658fa74 added hash support for row types. This also added
support for hashing anonymous record types, using the same approach
that the type cache uses for comparison support for record types: It
just reports that it works, but it might fail at run time if a
component type doesn't actually support the operation. We get away
with that for comparison because most types support that. But some
types don't support hashing, so the current state can result in
failures at run time where the planner chooses hashing over sorting,
whereas that previously worked if only sorting was an option.
We do, however, want the record hashing support for path tracking in
recursive unions, and the SEARCH and CYCLE clauses built on that. In
that case, hashing is the only plan option. So enable that, this
commit implements the following approach: The type cache does not
report that hashing is available for the record type. This undoes
that part of 01e658fa74. Instead, callers that require hashing no
matter what can override that result themselves. This patch only
touches the callers to make the aforementioned recursive query cases
work, namely the parse analysis of unions, as well as the hash_array()
function.
Bug: #17158
---
src/backend/parser/analyze.c | 23 ++++--
src/backend/rewrite/rewriteSearchCycle.c | 6 +-
src/backend/utils/adt/arrayfuncs.c | 37 +++++++++-
src/backend/utils/cache/typcache.c | 11 +--
src/include/parser/analyze.h | 2 +-
src/test/regress/expected/union.out | 90 ++++++++++++++----------
src/test/regress/sql/union.sql | 4 +-
7 files changed, 118 insertions(+), 55 deletions(-)
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 15669c8238..146ee8dd1e 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 require_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 require_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 (require_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..557bec5889 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"
@@ -3973,13 +3974,47 @@ 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",
format_type_be(element_type))));
fcinfo->flinfo->fn_extra = (void *) typentry;
}
+
+ /*
+ * 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)
+ {
+ MemoryContext oldcontext;
+ TypeCacheEntry *record_typentry;
+
+ oldcontext = MemoryContextSwitchTo(CacheMemoryContext);
+
+ /*
+ * Make fake type cache entry structure. Note that we can't just
+ * modify typentry, since that points directly into the type cache.
+ */
+ record_typentry = palloc(sizeof(*record_typentry));
+
+ /* fill in what we need below */
+ record_typentry->typlen = typentry->typlen;
+ record_typentry->typbyval = typentry->typbyval;
+ record_typentry->typalign = typentry->typalign;
+ fmgr_info(F_HASH_RECORD, &record_typentry->hash_proc_finfo);
+
+ MemoryContextSwitchTo(oldcontext);
+
+ typentry = record_typentry;
+
+ /* also save for future calls */
+ fcinfo->flinfo->fn_extra = (void *) typentry;
+ }
+
typlen = typentry->typlen;
typbyval = typentry->typbyval;
typalign = typentry->typalign;
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..a0f0bd38d7 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 require_hash);
#endif /* ANALYZE_H */
diff --git a/src/test/regress/expected/union.out b/src/test/regress/expected/union.out
index 75f78db8f5..dece7310cf 100644
--- a/src/test/regress/expected/union.out
+++ b/src/test/regress/expected/union.out
@@ -648,34 +648,37 @@ reset enable_hashagg;
set enable_hashagg to on;
explain (costs off)
select x from (values (row(1, 2)), (row(1, 3))) _(x) union select x from (values (row(1, 2)), (row(1, 4))) _(x);
- QUERY PLAN
------------------------------------------
- HashAggregate
- Group Key: "*VALUES*".column1
- -> Append
- -> Values Scan on "*VALUES*"
- -> Values Scan on "*VALUES*_1"
-(5 rows)
+ QUERY PLAN
+-----------------------------------------------
+ Unique
+ -> Sort
+ Sort Key: "*VALUES*".column1
+ -> Append
+ -> Values Scan on "*VALUES*"
+ -> Values Scan on "*VALUES*_1"
+(6 rows)
select x from (values (row(1, 2)), (row(1, 3))) _(x) union select x from (values (row(1, 2)), (row(1, 4))) _(x);
x
-------
- (1,4)
- (1,3)
(1,2)
+ (1,3)
+ (1,4)
(3 rows)
explain (costs off)
select x from (values (row(1, 2)), (row(1, 3))) _(x) intersect select x from (values (row(1, 2)), (row(1, 4))) _(x);
- QUERY PLAN
------------------------------------------------
- HashSetOp Intersect
- -> Append
- -> Subquery Scan on "*SELECT* 1"
- -> Values Scan on "*VALUES*"
- -> Subquery Scan on "*SELECT* 2"
- -> Values Scan on "*VALUES*_1"
-(6 rows)
+ QUERY PLAN
+-----------------------------------------------------
+ SetOp Intersect
+ -> Sort
+ Sort Key: "*SELECT* 1".x
+ -> Append
+ -> Subquery Scan on "*SELECT* 1"
+ -> Values Scan on "*VALUES*"
+ -> Subquery Scan on "*SELECT* 2"
+ -> Values Scan on "*VALUES*_1"
+(8 rows)
select x from (values (row(1, 2)), (row(1, 3))) _(x) intersect select x from (values (row(1, 2)), (row(1, 4))) _(x);
x
@@ -685,15 +688,17 @@ select x from (values (row(1, 2)), (row(1, 3))) _(x) intersect select x from (va
explain (costs off)
select x from (values (row(1, 2)), (row(1, 3))) _(x) except select x from (values (row(1, 2)), (row(1, 4))) _(x);
- QUERY PLAN
------------------------------------------------
- HashSetOp Except
- -> Append
- -> Subquery Scan on "*SELECT* 1"
- -> Values Scan on "*VALUES*"
- -> Subquery Scan on "*SELECT* 2"
- -> Values Scan on "*VALUES*_1"
-(6 rows)
+ QUERY PLAN
+-----------------------------------------------------
+ SetOp Except
+ -> Sort
+ Sort Key: "*SELECT* 1".x
+ -> Append
+ -> Subquery Scan on "*SELECT* 1"
+ -> Values Scan on "*VALUES*"
+ -> Subquery Scan on "*SELECT* 2"
+ -> Values Scan on "*VALUES*_1"
+(8 rows)
select x from (values (row(1, 2)), (row(1, 3))) _(x) except select x from (values (row(1, 2)), (row(1, 4))) _(x);
x
@@ -702,21 +707,28 @@ select x from (values (row(1, 2)), (row(1, 3))) _(x) except select x from (value
(1 row)
-- non-hashable type
--- With an anonymous row type, the typcache reports that the type is
--- hashable, but then it will fail at run time.
+-- With an anonymous row type, the typcache does not report that the
+-- type is hashable. (Otherwise, this would fail at execution time.)
explain (costs off)
select x from (values (row(100::money)), (row(200::money))) _(x) union select x from (values (row(100::money)), (row(300::money))) _(x);
- QUERY PLAN
------------------------------------------
- HashAggregate
- Group Key: "*VALUES*".column1
- -> Append
- -> Values Scan on "*VALUES*"
- -> Values Scan on "*VALUES*_1"
-(5 rows)
+ QUERY PLAN
+-----------------------------------------------
+ Unique
+ -> Sort
+ Sort Key: "*VALUES*".column1
+ -> Append
+ -> Values Scan on "*VALUES*"
+ -> Values Scan on "*VALUES*_1"
+(6 rows)
select x from (values (row(100::money)), (row(200::money))) _(x) union select x from (values (row(100::money)), (row(300::money))) _(x);
-ERROR: could not identify a hash function for type money
+ x
+-----------
+ ($100.00)
+ ($200.00)
+ ($300.00)
+(3 rows)
+
-- With a defined row type, the typcache can inspect the type's fields
-- for hashability.
create type ct1 as (f1 money);
diff --git a/src/test/regress/sql/union.sql b/src/test/regress/sql/union.sql
index ce22f34c71..ca8c9b4d12 100644
--- a/src/test/regress/sql/union.sql
+++ b/src/test/regress/sql/union.sql
@@ -218,8 +218,8 @@
-- non-hashable type
--- With an anonymous row type, the typcache reports that the type is
--- hashable, but then it will fail at run time.
+-- With an anonymous row type, the typcache does not report that the
+-- type is hashable. (Otherwise, this would fail at execution time.)
explain (costs off)
select x from (values (row(100::money)), (row(200::money))) _(x) union select x from (values (row(100::money)), (row(300::money))) _(x);
select x from (values (row(100::money)), (row(200::money))) _(x) union select x from (values (row(100::money)), (row(300::money))) _(x);
base-commit: e04267844a9bbf97c2e85c919b84dfe498ab0302
--
2.33.0
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-09-08 09:27 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 14+ messages in thread
From: Peter Eisentraut @ 2021-09-08 09:27 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On 02.09.21 10:15, Peter Eisentraut wrote:
> On 31.08.21 22:43, Tom Lane wrote:
>> I find variant 1 a bit cleaner, and safer. I'd rather default to
>> assuming that RECORD doesn't hash, when we don't have enough info
>> to be sure.
>
> Ok, here is a more polished patch for that.
committed
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-09-10 19:27 Tom Lane <tgl@sss.pgh.pa.us>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2021-09-10 19:27 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 02.09.21 10:15, Peter Eisentraut wrote:
>> On 31.08.21 22:43, Tom Lane wrote:
>>> I find variant 1 a bit cleaner, and safer. I'd rather default to
>>> assuming that RECORD doesn't hash, when we don't have enough info
>>> to be sure.
>> Ok, here is a more polished patch for that.
> committed
I apologize for not having found the time to review this before
it went in ... but what you did in hash_array is pretty awful:
/*
* 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)
{
MemoryContext oldcontext;
TypeCacheEntry *record_typentry;
oldcontext = MemoryContextSwitchTo(CacheMemoryContext);
/*
* Make fake type cache entry structure. Note that we can't just
* modify typentry, since that points directly into the type cache.
*/
record_typentry = palloc(sizeof(*record_typentry));
/* fill in what we need below */
record_typentry->typlen = typentry->typlen;
record_typentry->typbyval = typentry->typbyval;
record_typentry->typalign = typentry->typalign;
fmgr_info(F_HASH_RECORD, &record_typentry->hash_proc_finfo);
MemoryContextSwitchTo(oldcontext);
typentry = record_typentry;
}
fcinfo->flinfo->fn_extra = (void *) typentry;
}
The reason skink has been falling over since this went in is that
this kluge didn't bother to fill record_typentry->type_id, which
results in the next call seeing an undefined value in
if (typentry == NULL ||
typentry->type_id != element_type)
which most likely will cause it to allocate another dummy typcache
entry; lather, rinse, repeat for each call. But even with that
fixed, I do not think this is even a little bit acceptable, because
it will permanently leak a TypeCacheEntry plus subsidiary FmgrInfo data
for each query that uses hash_array.
Perhaps it'd work to put the phony entry into fcinfo->flinfo->fn_mcxt
instead of CacheMemoryContext.
BTW, skink's failure can be reproduced pretty quickly by running the
attached under valgrind.
regards, tom lane
create temp table graph0( f int, t int, label text );
insert into graph0 values
(1, 2, 'arc 1 -> 2'),
(1, 3, 'arc 1 -> 3'),
(2, 3, 'arc 2 -> 3'),
(1, 4, 'arc 1 -> 4'),
(4, 5, 'arc 4 -> 5');
with recursive search_graph(f, t, label) as (
select * from graph0 g
union distinct
select g.*
from graph0 g, search_graph sg
where g.f = sg.t
) search depth first by f, t set seq
select * from search_graph order by seq;
Attachments:
[text/plain] valgrind.sql (423B, ../../4092959.1631302070@sss.pgh.pa.us/2-valgrind.sql)
download | inline:
create temp table graph0( f int, t int, label text );
insert into graph0 values
(1, 2, 'arc 1 -> 2'),
(1, 3, 'arc 1 -> 3'),
(2, 3, 'arc 2 -> 3'),
(1, 4, 'arc 1 -> 4'),
(4, 5, 'arc 4 -> 5');
with recursive search_graph(f, t, label) as (
select * from graph0 g
union distinct
select g.*
from graph0 g, search_graph sg
where g.f = sg.t
) search depth first by f, t set seq
select * from search_graph order by seq;
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-09-14 14:39 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Peter Eisentraut @ 2021-09-14 14:39 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On 10.09.21 21:27, Tom Lane wrote:
> Perhaps it'd work to put the phony entry into fcinfo->flinfo->fn_mcxt
> instead of CacheMemoryContext.
>
> BTW, skink's failure can be reproduced pretty quickly by running the
> attached under valgrind.
Ok, the attached patch fixes the valgrind error.
Attachments:
[text/plain] 0001-Fix-hash_array.patch (0B, ../../2f0f926d-23c9-2ded-4032-dee85be733dc@enterprisedb.com/2-0001-Fix-hash_array.patch)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-09-14 15:02 Tom Lane <tgl@sss.pgh.pa.us>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2021-09-14 15:02 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 10.09.21 21:27, Tom Lane wrote:
>> Perhaps it'd work to put the phony entry into fcinfo->flinfo->fn_mcxt
>> instead of CacheMemoryContext.
> Ok, the attached patch fixes the valgrind error.
Looks roughly sane to me. I'm of two minds about whether you
ought to change the palloc to palloc0. We'd have taken much
longer to notice this problem if palloc0 had been used; but
from any standpoint other than "will valgrind catch it", it
seems like zeroing the fake typcache entry would be safer.
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: BUG #17158: Distinct ROW fails with Postgres 14
@ 2021-09-15 10:26 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 14+ messages in thread
From: Peter Eisentraut @ 2021-09-15 10:26 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; sait.nisanci@microsoft.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On 14.09.21 17:02, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> On 10.09.21 21:27, Tom Lane wrote:
>>> Perhaps it'd work to put the phony entry into fcinfo->flinfo->fn_mcxt
>>> instead of CacheMemoryContext.
>
>> Ok, the attached patch fixes the valgrind error.
>
> Looks roughly sane to me. I'm of two minds about whether you
> ought to change the palloc to palloc0. We'd have taken much
> longer to notice this problem if palloc0 had been used; but
> from any standpoint other than "will valgrind catch it", it
> seems like zeroing the fake typcache entry would be safer.
Yeah, pushed with the palloc0.
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2021-09-15 10:26 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-08-24 08:46 BUG #17158: Distinct ROW fails with Postgres 14 PG Bug reporting form <noreply@postgresql.org>
2021-08-24 09:55 ` David Rowley <dgrowleyml@gmail.com>
2021-08-24 14:03 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-08-24 15:05 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-08-24 21:25 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-08-24 22:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-08-27 10:36 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-08-31 20:43 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-09-02 08:15 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-09-08 09:27 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-09-10 19:27 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-09-14 14:39 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-09-14 15:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-09-15 10:26 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
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