From: Tom Lane <tgl@sss.pgh.pa.us>
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>
Subject: Re: BUG #17158: Distinct ROW fails with Postgres 14
In-reply-to: <333555.1629813806@sss.pgh.pa.us>
References: <17158-8a2ba823982537a4@postgresql.org>
 <CAApHDvp0w_5YY=7pxPkymtv7Oc0cGS=+jtiBvXrfcM6gLjT5Pg@mail.gmail.com>
 <333555.1629813806@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 24 Aug 2021 10:03:26 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <343746.1629817538.1@sss.pgh.pa.us>
Date: Tue, 24 Aug 2021 11:05:38 -0400
Message-ID: <343747.1629817538@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/343747.1629817538%40sss.pgh.pa.us>
Precedence: bulk

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