public inbox for [email protected]  

help / color / mirror / Atom feed

From: Andrey Borodin <[email protected]>
To: Arseniy Mukhin <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: amcheck support for BRIN indexes
Date: Mon, 16 Jun 2025 22:11:31 +0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAE7r3MLmGA_DbUFYxNp4L7ODwDyLQ2u_vfdTSXj-QJu79B4iHg@mail.gmail.com>
References: <CAE7r3MKZkMwuEeFU5M73Jk3yp+2CBr41a3tfvb3CHym+ysAVEA@mail.gmail.com>
	<CAE7r3MLmGA_DbUFYxNp4L7ODwDyLQ2u_vfdTSXj-QJu79B4iHg@mail.gmail.com>

Hi!

Nice feature!

> On 8 Jun 2025, at 17:39, Arseniy Mukhin <[email protected]> wrote:
> 
> <v2-0001-brin-refactoring.patch>


+#define BRIN_MAX_PAGES_PER_RANGE	131072

I'd personally prefer some words on where does this limit come from. I'm not a BRIN pro, just curious.
Or, at least, maybe we can use a form 128 * 1024, if it's just a sane limit.



> On 8 Jun 2025, at 17:39, Arseniy Mukhin <[email protected]> wrote:
> <v2-0002-amcheck-brin-support.patch>


A bit more detailed commit message would be very useful.

The test coverage is very decent. The number of possible corruptions in tests is impressive. I don't really have an experience with BRIN to say how different they are, but I want to ask if you are sure that these types of corruption will be portable across big-endian machines and such stuff.

Copyright year in all new files should be 2025.

Some documentation about brin_index_check() would be handy, especially about its parameters. Perhaps, somewhere near gin_index_check() in amcheck.sgml.

brin_check_ereport() reports ERRCODE_DATA_CORRUPTED. We should distinguish between ERRCODE_INDEX_CORRUPTED which is a stormbringer and ERRCODE_DATA_CORRUPTED which is an actual disaster.

If it's not very difficult - it would be great to use read_stream infrastructure. See btvacuumscan() in nbtree.c calling read_stream_begin_relation() for example. We cannot use it in logical scans in B-tree\GiST\GIN, but maybe BRIN can take some advantage of this new shiny technology.

+		state->revmapbuf = ReadBufferExtended(idxrel, MAIN_FORKNUM, state->revmapBlk, RBM_NORMAL,
+											  state->checkstrategy);
+		LockBuffer(state->revmapbuf, BUFFER_LOCK_SHARE);
// usage of state->revmapbuf
+		LockBuffer(state->revmapbuf, BUFFER_LOCK_UNLOCK);
// more usage of state->revmapbuf
+		ReleaseBuffer(state->revmapbuf);

I hope you know what you are doing. BRIN concurrency is not known to me at all.


That's all for first pass through patches. Thanks for working on it!


Best regards, Andrey Borodin.

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]
  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