>
> 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>
v6-0001-Add-option-for-amcheck-and-pg_amcheck-to-check-un.patch
Description: Binary data
