From: | Andrey Borodin <x4mmm(at)yandex-team(dot)ru> |
---|---|
To: | Arseniy Mukhin <arseniy(dot)mukhin(dot)dev(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: amcheck support for BRIN indexes |
Date: | 2025-06-16 17:11:31 |
Message-ID: | [email protected] |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi!
Nice feature!
> On 8 Jun 2025, at 17:39, Arseniy Mukhin <arseniy(dot)mukhin(dot)dev(at)gmail(dot)com> 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 <arseniy(dot)mukhin(dot)dev(at)gmail(dot)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 = 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.
From | Date | Subject | |
---|---|---|---|
Next Message | Nathan Bossart | 2025-06-16 17:21:32 | Re: Per-role disabling of LEAKPROOF requirements for row-level security? |
Previous Message | Peter Geoghegan | 2025-06-16 16:46:29 | Returning nbtree posting list TIDs in DESC order during backwards scans |