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

> On Dec 20, 2021, at 7:37 AM, Pavel Borisov <pashkin.elfe@gmail.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