Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tnmCN-001umR-Uu for pgsql-committers@arkaria.postgresql.org; Thu, 27 Feb 2025 22:17:45 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1tnmCO-007402-MB for pgsql-committers@arkaria.postgresql.org; Thu, 27 Feb 2025 22:17:43 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tnmCO-0073zI-9E for pgsql-committers@lists.postgresql.org; Thu, 27 Feb 2025 22:17:43 +0000 Received: from mail-oi1-x236.google.com ([2607:f8b0:4864:20::236]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1tnmCJ-0003Ew-2v for pgsql-committers@lists.postgresql.org; Thu, 27 Feb 2025 22:17:41 +0000 Received: by mail-oi1-x236.google.com with SMTP id 5614622812f47-3f1c94936c5so920003b6e.1 for ; Thu, 27 Feb 2025 14:17:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb.com; s=google; t=1740694659; x=1741299459; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=hc/sbupTYV70FRbK5xEj21DowbvLGZkhBoclPZ+YrcQ=; b=GreZiOFtQO/kOjK9kVmQj6X6XK/sZ9f2ant5dFLG4lG/gkPWtcsqp7v1aIpRVgA8Q7 tHQYG8Db0SM+Y4fixv7ysQgHuezkjuCYG7c3OxzlMMYRZaLQa1Wcf7BDh4OqBT9Jlefs hpekN2s0EJi2TaAbzEJy5gtMzVzmbfKgdyrtEfQ+RRt8h0JzhleZnnyLwM2Pg4iloAEN Lqtz5Cw1wd07kGsJ7Jkq0V2WRIs+VDIBY0do5Ipv8KzCiZeADwSqpXGrAV/p2exhn9/6 Em4xC0++iGIiqsJGvUTsiJlHy0aI89dslr6i0odSW/ZUW23IOP1y0fLuu4AL0Yvc1RUn wklQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740694659; x=1741299459; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=hc/sbupTYV70FRbK5xEj21DowbvLGZkhBoclPZ+YrcQ=; b=UtJRXguaVEpXqj3ixL7ibyZYi/S4u07hMUu3L5LOnzcpYwlgRbp3yt051T33cO6TLk vpkh3X6GsnJW8F9BAXMLkYXxKo7HUZq6v21/MpyWWB8+DdvMWgzSO/R+/1I/ASmpYxXe U4dzfC+/7mhWFIidXTS70yyFhvWhKOrerARiJgkiXe2mRIrthfNKREyS8n9iJIroVF5+ XXY3HD3xMEC7yDNcc0WpwD44bqEfbjgS7cZIyQRcvuRWAT8HHT2HVyIw+wNtrg2uWPTT q2uYSi8mF9mi6IGF9uRr3nkleNtxgZkjeGZEoDI8lEcSsdMDSEm8hEv9BLTXfsUz890C 1BGA== X-Forwarded-Encrypted: i=1; AJvYcCWpBLUVUt/ZzlOu1xHuuEmxg/cR9KajiWfLaEHFJofgMOtXaFIslswYlkhg2zJ6+4fhkwbP0mt56SWfhxTJwBcN@lists.postgresql.org X-Gm-Message-State: AOJu0YzdeXG1KKaoULjzH77F1lbV3RtJQk375oNUnkmeYKSTdcbpg8H5 SBiMcT2+AmlMoy7Mdoe29bcrGBgsfq+zrPZ/Ze+2Dn2IZaoC1Ww5ZA0NiBKCCJ0QUGpYbOP0BKK ww7JMdvYQ57fSDHswJIhxr9coRhRVClFyyk+U X-Gm-Gg: ASbGncsYpz/9pdC6VnJwd3R3n2/J4sIEXrBUIK3zy1GSTD1wwO+XMB7hLZG8bT+a7zX ikOgdTSYEscs9nKGhhAMqEtYGXW+ZAYmfjHXE7cI+xlRiVBR5Dah/2f66mFFxCw8scAgvtBLQ3w BHHWKItv4= X-Google-Smtp-Source: AGHT+IGGfr2QMInv/8H6lMHAH43Ve/rtojF6RnI0xKW7w70Bs90c/Xt5qXBVqSGuEVVomOt6mQDnvDsBBoxW0LPgcIY= X-Received: by 2002:a05:6808:2e4a:b0:3f1:e75c:5aa8 with SMTP id 5614622812f47-3f54ed5250fmr3297478b6e.3.1740694659010; Thu, 27 Feb 2025 14:17:39 -0800 (PST) MIME-Version: 1.0 References: <2557074.1740673653@sss.pgh.pa.us> In-Reply-To: <2557074.1740673653@sss.pgh.pa.us> From: Mark Dilger Date: Thu, 27 Feb 2025 14:17:27 -0800 X-Gm-Features: AQ5f1JojNDdTsQ4QwAVTGcmXuGjaA2TjQYF81-WWRbliKzMk4nCD4FIxHx0K0Q0 Message-ID: Subject: Re: pgsql: Generalize hash and ordering support in amapi To: Tom Lane Cc: Peter Eisentraut , pgsql-committers@lists.postgresql.org Content-Type: multipart/alternative; boundary="000000000000d0303c062f270edf" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000d0303c062f270edf Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Feb 27, 2025 at 8:27=E2=80=AFAM Tom Lane wrote: > Peter Eisentraut 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 =3D=3D BTREE_AM_OID || > - op_form->amopmethod =3D=3D HASH_AM_OID) > + if (amroutine->amcancrosscompare) > It seems you are right. hashhandler()'s amroutine should have this true, also. > > 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? > The comments in amapi.h seem to have gotten shortened since v19 of the patch which had them as: + /* + * Does AM support hashing the indexed column's value, including providing + * all hash semantics functions for HASHSTANDARD_PROC and HASHEXTENDED_PROC + * conforming to the same calling conventions as those of the hash AM? + */ + bool amcanhash; + /* + * Does AM have equality semantics that are compatible across all equality + * operators within an operator family? + */ + bool amcancrosscompare; The logic in equality_ops_are_compatible() was trusting that equality operators found in an opfamily for btree or hash were ok, but not trusting operators found in opfamilies of other AMs. Now, after the patch, other AMs can be marked as suitable. That's really the core of what the flag means: "Can the system trust that equality operators found in opfamilies of the AM are well-behaved", or something like that. I agree that this should really be more a feature of an opfamily than an AM, but the system already does it on AM-level granularity, and I don't think it's the responsibility of this patch to rearchitect all that. > 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 agree these comments need updating. =E2=80=94 Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company --000000000000d0303c062f270edf Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

On Thu, Feb 27, 202= 5 at 8:27=E2=80=AFAM Tom Lane <tgl@= sss.pgh.pa.us> wrote:
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.=C2=A0 A field amcanord= er
> already existed, this uses it more widely.=C2=A0 Fields amcanhash and<= br> > 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:

-=C2=A0 =C2=A0 =C2=A0 =C2=A0/* must be btree or hash */
-=C2=A0 =C2=A0 =C2=A0 =C2=A0if (op_form->amopmethod =3D=3D BTREE_AM_OID = ||
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0op_form->amopmethod =3D=3D HAS= H_AM_OID)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (amroutine->amcancrosscompare)

It seems you are right. =C2=A0hashhandler()'s= amroutine=C2=A0should have this true, also.
=C2=A0

More generally, I think that "amcancrosscompare" is a horribly underspecified and misleadingly-named concept.=C2=A0 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.=C2=A0 So what does that flag *really* mean?

The comments in amapi.h seem to have gotten sho= rtened since v19 of the patch which had them as:

+= =C2=A0 /*
+ =C2=A0 =C2=A0* Does AM support hashing the indexed column&#= 39;s value, including providing
+ =C2=A0 =C2=A0* all hash semantics func= tions for HASHSTANDARD_PROC and HASHEXTENDED_PROC
+ =C2=A0 =C2=A0* confo= rming to the same calling conventions as those of the hash AM?
+ =C2=A0 = =C2=A0*/
+ =C2=A0 bool =C2=A0 =C2=A0 =C2=A0 =C2=A0amcanhash;
+ =C2=A0= /*
+ =C2=A0 =C2=A0* Does AM have equality semantics that are compatible= across all equality
+ =C2=A0 =C2=A0* operators within an operator famil= y?
+ =C2=A0 =C2=A0*/
+ =C2=A0 bool =C2=A0 =C2=A0 =C2=A0 =C2=A0amcancr= osscompare;

The logic in equality_ops_are_comp= atible() was trusting that equality operators found in an opfamily=C2=A0for= btree or hash were ok, but not trusting operators found in opfamilies=C2= =A0of other AMs.=C2=A0 Now, after the patch, other AMs can be marked as sui= table.=C2=A0 That's really the core of what the flag means: =C2=A0"= ;Can the system trust that equality operators found in opfamilies of the AM= are well-behaved", or something like that.=C2=A0 I agree that this sh= ould really be more a feature of an opfamily than an AM, but the system alr= eady does it on AM-level granularity, and I don't think it's the re= sponsibility of this patch to rearchitect all that.


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

=C2=A0* This is trivially true if they are the same operator.=C2=A0 Otherwi= se,
=C2=A0* we look to see if they can be found in the same btree or hash opfam= ily.

=C2=A0* This is trivially true if they are the same operator.=C2=A0 Otherwi= se,
=C2=A0* we look to see if they can be found in the same btree opfamily.
=

I agree these comments need updating.
=E2=80=94
Mark Dilg= er
Ent= erpriseDB:=C2=A0http://w= ww.enterprisedb.com
The Enterprise PostgreSQL Company
--000000000000d0303c062f270edf--