From: Tom Lane <tgl@sss.pgh.pa.us>
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>
Subject: Re: BUG #17158: Distinct ROW fails with Postgres 14
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>
Comments: In-reply-to Peter Eisentraut <peter.eisentraut@enterprisedb.com>
	message dated "Wed, 08 Sep 2021 11:27:11 +0200"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <4092917.1631302019.0@sss.pgh.pa.us>
Date: Fri, 10 Sep 2021 15:27:50 -0400
Message-ID: <4092959.1631302070@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/4092959.1631302070%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4092917.1631302019.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

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 =3D=3D RECORDOID)
        {
            MemoryContext oldcontext;
            TypeCacheEntry *record_typentry;

            oldcontext =3D MemoryContextSwitchTo(CacheMemoryContext);

            /*
             * Make fake type cache entry structure.  Note that we can't j=
ust
             * modify typentry, since that points directly into the type c=
ache.
             */
            record_typentry =3D palloc(sizeof(*record_typentry));

            /* fill in what we need below */
            record_typentry->typlen =3D typentry->typlen;
            record_typentry->typbyval =3D typentry->typbyval;
            record_typentry->typalign =3D typentry->typalign;
            fmgr_info(F_HASH_RECORD, &record_typentry->hash_proc_finfo);

            MemoryContextSwitchTo(oldcontext);

            typentry =3D record_typentry;
        }

        fcinfo->flinfo->fn_extra =3D (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 =3D=3D NULL ||
        typentry->type_id !=3D 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


------- =_aaaaaaaaaa0
Content-Type: text/plain; name="valgrind.sql"; charset="us-ascii"
Content-ID: <4092917.1631302019.2@sss.pgh.pa.us>
Content-Description: valgrind.sql


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;

------- =_aaaaaaaaaa0--