Horton

Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.

Mark Dilger <mark.dilger@enterprisedb.com>

View thread
From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Pavel Borisov <pashkin.elfe@gmail.com>
Cc: David Steele <david@pgmasters.net>, Postgres hackers <pgsql-hackers@lists.postgresql.org>, Peter Geoghegan <pg@bowt.ie>, Maxim Orlov <m.orlov@postgrespro.ru>, lubennikovaav@gmail.com
Date: 2021-12-23T16:31:29Z
Lists: pgsql-hackers

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: Optimize speed of checking for unique constraint violation

  2. amcheck: Report an error when the next page to a leaf is not a leaf

  3. amcheck: Don't load the right sibling page into BtreeCheckState

  4. amcheck: Refactoring the storage of the last visible entry

  5. Teach contrib/amcheck to check the unique constraint violation

  6. Add macros in hash and btree AMs to get the special area of their pages


> On Dec 22, 2021, at 12:01 AM, Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> 
> Thank you, Mark!
> 
> In v6 (PFA) I've made the changes on your advice i.e.
> 
> - pg_amcheck with --checkunique option will ignore uniqueness check (with a warning) if amcheck version in a db is <1.4 and doesn't support the feature.

Ok.

+           int         vmaj = 0,
+                       vmin = 0,
+                       vrev = 0;
+           const char *amcheck_version = pstrdup(PQgetvalue(result, 0, 1));
+
+           sscanf(amcheck_version, "%d.%d.%d", &vmaj, &vmin, &vrev);

The pstrdup is unnecessary but harmless.

> - fixed unnecessary drop table in regression

Ok.

> - use the existing table for uniqueness check in 005_opclass_damage.pl

It appears you still create a new table, bttest_unique, rather than using the existing table int4tbl.  That's fine.

> - added tests into 003_check.pl . It is only smoke test that just verifies new functions.

+
+$node->command_checks_all(
+   [
+       @cmd, '-s', 's1', '-i', 't1_btree', '--parent-check',
+       '--checkunique', 'db1'
+   ],
+   2,
+   [$index_missing_relation_fork_re],
+   [$no_output_re],
+   'pg_amcheck smoke test --parent-check');
+
+$node->command_checks_all(
+   [
+       @cmd, '-s', 's1', '-i', 't1_btree', '--heapallindexed',
+       '--rootdescend', '--checkunique',  'db1'
+   ],
+   2,
+   [$index_missing_relation_fork_re],
+   [$no_output_re],
+   'pg_amcheck smoke test --heapallindexed --rootdescend');
+
+$node->command_checks_all(
+   [ @cmd, '--checkunique', '-d', 'db1', '-d', 'db2', '-d', 'db3', '-S', 's*' ],
+   0, [$no_output_re], [$no_output_re],
+   'pg_amcheck excluding all corrupt schemas');
+

You have borrowed the existing tests but forgot to change their names.  (The last line of each check is the test name, such as 'pg_amcheck smoke test --parent-check'.)  Please make each test name unique.

> - added test contrib/amcheck/t/004_verify_nbtree_unique.pl it is more extensive test based on opclass damage which was intended to be main test for amcheck, but which I've forgotten to add to commit in v5.
> 005_opclass_damage.pl test, which you've seen in v5 is largely based on first part of 004_verify_nbtree_unique.pl (with the later calling pg_amcheck, and the former calling bt_index_check(), bt_index_parent_check() )

Ok.

> - added forgotten upgrade script amcheck--1.3--1.4.sql (from v4)

Ok.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company