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

>
> 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.
>

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.
- fixed unnecessary drop table in regression
- use the existing table for uniqueness check in 005_opclass_damage.pl
- added tests into 003_check.pl . It is only smoke test that just verifies
new functions.
- 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() )
- added forgotten upgrade script amcheck--1.3--1.4.sql (from v4)

You are welcome with more considerations.

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>