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

> On Dec 20, 2021, at 7:37 AM, Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com> wrote:
>
> The patch is pgindented and rebased on the current PG master code.

Thank you, Pavel.

The tests in check_btree.sql no longer create a bttest_unique table, so the DROP TABLE is surplusage:

+DROP TABLE bttest_unique;
+ERROR: table "bttest_unique" does not exist

The changes in pg_amcheck.c to pass the new checkunique parameter will likely need to be based on a amcheck version check. The implementation of prepare_btree_command() in pg_amcheck.c should be kept compatible with older versions of amcheck, because it connects to remote servers and you can't know in advance that the remote servers are as up-to-date as the machine where pg_amcheck is installed. I'm thinking specifically about this change:

@@ -871,7 +877,8 @@ prepare_btree_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn)
if (opts.parent_check)
appendPQExpBuffer(sql,
"SELECT %s.bt_index_parent_check("
- "index := c.oid, heapallindexed := %s, rootdescend := %s)"
+ "index := c.oid, heapallindexed := %s, rootdescend := %s, "
+ "checkunique := %s)"
"\nFROM pg_catalog.pg_class c, pg_catalog.pg_index i "
"WHERE c.oid = %u "
"AND c.oid = i.indexrelid "

If the user calls pg_amcheck with --checkunique, and one or more remote servers have an amcheck version < 1.4, at a minimum you'll need to avoid calling bt_index_parent_check with that parameter, and probably also you'll either need to raise a warning or perhaps an error telling the user that such a check cannot be performed.

You've forgotten to include contrib/amcheck/amcheck--1.3--1.4.sql in the v5 patch, resulting in a failed install:

/usr/bin/install -c -m 644 ./amcheck--1.3--1.4.sql ./amcheck--1.2--1.3.sql ./amcheck--1.1--1.2.sql ./amcheck--1.0--1.1.sql ./amcheck--1.0.sql '/Users/mark.dilger/hydra/unique_review.5/tmp_install/Users/mark.dilger/pgtest/test_install/share/postgresql/extension/'
install: ./amcheck--1.3--1.4.sql: No such file or directory
make[2]: *** [install] Error 71
make[1]: *** [checkprep] Error 2

Using the one from the v4 patch fixes the problem. Please include this file in v6, to simplify the review process.

The changes to t/005_opclass_damage.pl look ok. The creation of a new table for the new test seems unnecessary, but only problematic in that it makes the test slightly longer to read. I recommend changing the test to use the same table that the prior test uses, but that is just a recommendation, not a requirement.

You should add coverage for --checkunique to t/003_check.pl.

You should add coverage for multiple PostgreSQL::Test::Cluster instances running differing versions of amcheck, perhaps some on version 1.3 and some on version 1.4. Then test that the --checkunique option works adequately.

I have not reviewed the changes to verify_nbtree.c. I'll leave that to Peter.

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