From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
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, 10 Sep 2021 15:27:50 -0400
Message-ID: <4092959.1631302070@sss.pgh.pa.us> (raw)
In-Reply-To: <4519d7da-7402-cff3-d577-0bc8511e0c9a@enterprisedb.com>
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>
<c182f5c8-fdf3-80a0-fa43-4ed7e87d4d47@enterprisedb.com>
<2021952.1630442634@sss.pgh.pa.us>
<7fb17b3d-13dc-fc28-17ce-9c302d676bd8@enterprisedb.com>
<4519d7da-7402-cff3-d577-0bc8511e0c9a@enterprisedb.com>
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:valgrind.sql (text/plain, 423B) download
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;
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: tgl@sss.pgh.pa.us, peter.eisentraut@enterprisedb.com, dgrowleyml@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17158: Distinct ROW fails with Postgres 14
In-Reply-To: <4092959.1631302070@sss.pgh.pa.us>
* 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