Feedback-ID: ie3de48e3:Fastmail
Date: Tue, 5 Aug 2025 13:21:51 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Tomas Vondra <tomas@vondra.me>
Cc: Arseniy Mukhin <arseniy.mukhin.dev@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: amcheck support for BRIN indexes
Message-ID: <202508051121.fy6nhwsd4l26@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <67115399-4799-4c3b-bd17-d43c98099919@vondra.me>
Archived-At: 
 <https://www.postgresql.org/message-id/202508051121.fy6nhwsd4l26%40alvherre.pgsql>
Precedence: bulk

On 2025-Jul-08, Tomas Vondra wrote:

> On 7/8/25 14:40, Arseniy Mukhin wrote:

> > Thank you for the feedback! I agree with the benefits. Speaking of
> > (с), it seems most of the time to be really trivial to build such a
> > ScanKey, but not every opclass supports '=' operator. amcheck should
> > handle these cases somehow then. I see two options here. The first is
> > to not provide  'heap all indexed' check for such opclasses, which is
> > sad because even one core opclass (box_inclusion_ops) doesn't support
> > '=' operator, postgis brin opclasses don't support it too AFAICS. The
> > second option is to let the user define which operator to use during
> > the check, which, I think, makes user experience much worse in this
> > case. So both options look not good from the user POV as for me, so I
> > don't know. What do you think about it?
> > 
> > And should I revert the patchset to the consistent function version then?
> 
> Yeah, that's a good point. The various opclasses may support different
> operators, and we don't know which "strategy" to fill into the scan key.
> Minmax needs BTEqualStrategyNumber, inclusion RTContainsStrategyNumber,
> and so on.

Hmm, maybe we can make the operator argument to the function an optional
argument.  Then, if it's not given, use equality for the cases where
that works; if equality doesn't work for the column in that opclass,
throw an error to request an operator.  That way we support the most
common case in the easy way, and for the other cases the user has to
work a little harder -- but I think it's not too bad.

I think you should have tests with indexes on more than one column.

This syntax looks terrible
  SELECT brin_index_check('brintest_idx'::REGCLASS, true, true, '{"@>"}');

the thingy at the end looks like '90s modem line noise.  Can we maybe
use a variadic argument, so that if you have multiple indexed columns
you specify the operators in separate args somehow and avoid the quoting
and array decoration?  I imagine something like

  SELECT brin_index_check('brintest_idx'::REGCLASS, true, true, '@>', '=');
or whatever.  (To think about: if I want '=' to be omitted, but I have
the second column using a type that doesn't support the '=', what's a
good syntax to use?)


Regarding 0003: I think the new function should be just
CheckIndexCheckXMin(Relation idxrel, Snapshot snap)
and make the caller responsible for the snapshot handling.  Otherwise
you end up in the weird situation in 0004 where you have to do
  UnregisterSnapshot(RegisterSnapshotAndDoStuff())
instead of the more ordinary
  RegisterSnapshot()
  CheckIndexCheckXMin()
  UnregisterSnapshot()


You need an amcheck.sgml update for the new function.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/