public inbox for [email protected]  

help / color / mirror / Atom feed

From: Álvaro Herrera <[email protected]>
To: Tomas Vondra <[email protected]>
Cc: Arseniy Mukhin <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: amcheck support for BRIN indexes
Date: Tue, 5 Aug 2025 13:21:51 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

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/

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: amcheck support for BRIN indexes
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox