Horton
Menu

Re: Amcheck verification of GiST and GIN

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-06-22T23:29:12Z
Lists: pgsql-hackers 
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

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. amcheck: Fix posting tree checks in gin_index_check()

  2. amcheck: Fix parent key check in gin_index_check()

  3. amcheck: Fix checks of entry order for GIN indexes

  4. amcheck: Remove unused GinScanItem->parentlsn field

  5. amcheck: Test gin_index_check on a multicolumn index

  6. Remove incidental md5() function use from test

  7. amcheck: Add a GIN index to the CREATE INDEX CONCURRENTLY tests

  8. amcheck: Add a test with GIN index on JSONB data

  9. amcheck: Fix indentation in verify_gin.c

  10. amcheck: Add gin_index_check() to verify GIN index

  11. amcheck: Move common routines into a separate module

  12. Fix grammar in GIN README

  13. Avoid amcheck inline compression false positives.