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