From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Eisentraut <peter@eisentraut.org>
cc: pgsql-committers@lists.postgresql.org,
        Mark Dilger <mark.dilger@enterprisedb.com>
Subject: Re: pgsql: Generalize hash and ordering support in amapi
In-reply-to: <E1tngY6-0000UL-2n@gemulon.postgresql.org>
References: <E1tngY6-0000UL-2n@gemulon.postgresql.org>
Comments: In-reply-to Peter Eisentraut <peter@eisentraut.org>
	message dated "Thu, 27 Feb 2025 16:15:46 +0000"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2557073.1740673653.1@sss.pgh.pa.us>
Date: Thu, 27 Feb 2025 11:27:33 -0500
Message-ID: <2557074.1740673653@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2557074.1740673653%40sss.pgh.pa.us>
Precedence: bulk

Peter Eisentraut <peter@eisentraut.org> writes:
> Generalize hash and ordering support in amapi
> Stop comparing access method OID values against HASH_AM_OID and
> BTREE_AM_OID, and instead check the IndexAmRoutine for an index to see
> if it advertises its ability to perform the necessary ordering,
> hashing, or cross-type comparing functionality.  A field amcanorder
> already existed, this uses it more widely.  Fields amcanhash and
> amcancrosscompare are added for the other purposes.

AFAICS, this patch sets amcancrosscompare true only for btree,
which means this change to equality_ops_are_compatible is surely wrong:

-       /* must be btree or hash */
-       if (op_form->amopmethod == BTREE_AM_OID ||
-           op_form->amopmethod == HASH_AM_OID)
+       if (amroutine->amcancrosscompare)

More generally, I think that "amcancrosscompare" is a horribly
underspecified and misleadingly-named concept.  Most of our
AMs are capable of supporting cross-type operators, though for
some it's more about what the per-opclass infrastructure can do
than what the AM does.  So what does that flag *really* mean?

I also object strongly to the fact that the comments for
equality_ops_are_compatible and comparison_ops_are_compatible
were not modified:

 * This is trivially true if they are the same operator.  Otherwise,
 * we look to see if they can be found in the same btree or hash opfamily.

 * This is trivially true if they are the same operator.  Otherwise,
 * we look to see if they can be found in the same btree opfamily.

I do not think this was in shape to be committed.  For stuff
like this, clarity of thought and precision of specification
are critical, and this patch has neither.

			regards, tom lane