From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> |
Cc: | David Rowley <dgrowleyml(at)gmail(dot)com>, sait(dot)nisanci(at)microsoft(dot)com, PostgreSQL mailing lists <pgsql-bugs(at)lists(dot)postgresql(dot)org> |
Subject: | Re: BUG #17158: Distinct ROW fails with Postgres 14 |
Date: | 2021-09-10 19:27:50 |
Message-ID: | 4092959.1631302070@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs |
Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)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
Attachment | Content-Type | Size |
---|---|---|
valgrind.sql | text/plain | 424 bytes |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2021-09-11 05:38:47 | Re: BUG #17186: In connect.c, the lock connections_mutex is not correctly released(Line 463) at the return(Line 522) |
Previous Message | David G. Johnston | 2021-09-10 13:02:04 | Re: PG12 → PG13 database conversion anomaly: ERROR: function kepler_start_3(double precision, double precision) does not exist |