Re: Amcheck verification of GiST and GIN

Hi,

I think having amcheck for more indexes is great.

On 2022-06-22 20:40:56 +0300, Andrey Borodin wrote:
>> diff --git a/contrib/amcheck/amcheck.c b/contrib/amcheck/amcheck.c
> new file mode 100644
> index 0000000000..7a222719dd
> --- /dev/null
> +++ b/contrib/amcheck/amcheck.c
> @@ -0,0 +1,187 @@
> +/*-------------------------------------------------------------------------
> + *
> + * amcheck.c
> + * Utility functions common to all access methods.

This'd likely be easier to read if the reorganization were split into its own
commit.

I'd also split gin / gist support. It's a large enough patch that that imo
makes reviewing easier.

> +void amcheck_lock_relation_and_check(Oid indrelid, IndexCheckableCallback checkable,
> + IndexDoCheckCallback check, LOCKMODE lockmode, void *state)

Might be worth pgindenting - the void for function definitions (but not for
declarations) is typically on its own line in PG code.

> +static GistCheckState
> +gist_init_heapallindexed(Relation rel)
> +{
> + int64 total_pages;
> + int64 total_elems;
> + uint64 seed;
> + GistCheckState result;
> +
> + /*
> + * Size Bloom filter based on estimated number of tuples in index
> + */
> + total_pages = RelationGetNumberOfBlocks(rel);
> + total_elems = Max(total_pages * (MaxOffsetNumber / 5),
> + (int64) rel->rd_rel->reltuples);
> + /* Generate a random seed to avoid repetition */
> + seed = pg_prng_uint64(&pg_global_prng_state);
> + /* Create Bloom filter to fingerprint index */
> + result.filter = bloom_create(total_elems, maintenance_work_mem, seed);
> +
> + /*
> + * Register our own snapshot
> + */
> + result.snapshot = RegisterSnapshot(GetTransactionSnapshot());

FWIW, comments like this, that just restate exactly what the code does, are
imo not helpful. Also, there's a trailing space :)

Greetings,

Andres Freund