Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.600.51.1.1\))
Subject: Re: amcheck support for BRIN indexes
From: Andrey Borodin <x4mmm@yandex-team.ru>
In-Reply-To: 
 <CAE7r3MLmGA_DbUFYxNp4L7ODwDyLQ2u_vfdTSXj-QJu79B4iHg@mail.gmail.com>
Date: Mon, 16 Jun 2025 22:11:31 +0500
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <0C842FA2-8ACF-4C81-9F0E-F0BDB479AC28@yandex-team.ru>
References: 
 <CAE7r3MKZkMwuEeFU5M73Jk3yp+2CBr41a3tfvb3CHym+ysAVEA@mail.gmail.com>
 <CAE7r3MLmGA_DbUFYxNp4L7ODwDyLQ2u_vfdTSXj-QJu79B4iHg@mail.gmail.com>
To: Arseniy Mukhin <arseniy.mukhin.dev@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/0C842FA2-8ACF-4C81-9F0E-F0BDB479AC28%40yandex-team.ru>
Precedence: bulk

Hi!

Nice feature!

> On 8 Jun 2025, at 17:39, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> =
wrote:
>=20
> <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 <arseniy.mukhin.dev@gmail.com> =
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 =3D 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.=