Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Subject: Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree
 index.
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: 
 <CALT9ZEF=1q-mJiHV6E9e0cQmOYur0vtSX387KLZGs5U3me7vxw@mail.gmail.com>
Date: Mon, 20 Dec 2021 10:02:18 -0800
Cc: David Steele <david@pgmasters.net>,
 Anastasia Lubennikova <a.lubennikova@postgrespro.ru>,
 Postgres hackers <pgsql-hackers@lists.postgresql.org>,
 Peter Geoghegan <pg@bowt.ie>,
 Maxim Orlov <m.orlov@postgrespro.ru>
Content-Transfer-Encoding: quoted-printable
Message-Id: <D0DF2FEB-0060-488A-95B2-E102088DB7C4@enterprisedb.com>
References: 
 <CALT9ZEHRn5xAM5boga0qnrCmPV52bScEK2QnQ1HmUZDD301JEg@mail.gmail.com>
 <E91B88AB-E3A1-42A7-9BE2-40805AE5F35C@enterprisedb.com>
 <CALT9ZEFYsYPms0SPp4ZxpFCOBQbgE+iopoVKzU8Pni8Y6+RKtQ@mail.gmail.com>
 <6A958AE0-CFB9-4B1F-8EF8-BF6E729AFB59@enterprisedb.com>
 <CALT9ZEGgKAXDfVaAjBjBf4e6-L_4NVQsJ+7gzHd0JLwNxjXTFA@mail.gmail.com>
 <6FED9A6F-9AB3-4711-A5F7-55535ADF736B@enterprisedb.com>
 <CALT9ZEFMb96zVafrt7sUPrWYMDQVngaPcXuVqXCfawA2cfWwxw@mail.gmail.com>
 <4A5C6A8E-8C8C-4D9B-8FFA-DFAA697E7A53@enterprisedb.com>
 <a3e98907-dbca-f6ab-c985-427dee72721f@pgmasters.net>
 <CALT9ZEF=1q-mJiHV6E9e0cQmOYur0vtSX387KLZGs5U3me7vxw@mail.gmail.com>
To: Pavel Borisov <pashkin.elfe@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/D0DF2FEB-0060-488A-95B2-E102088DB7C4%40enterprisedb.com>
Precedence: bulk


> On Dec 20, 2021, at 7:37 AM, Pavel Borisov <pashkin.elfe@gmail.com> =
wrote:
>=20
> 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 :=3D c.oid, heapallindexed :=3D %s, =
rootdescend :=3D %s)"
+                         "index :=3D c.oid, heapallindexed :=3D %s, =
rootdescend :=3D %s, "
+                         "checkunique :=3D %s)"
                          "\nFROM pg_catalog.pg_class c, =
pg_catalog.pg_index i "
                          "WHERE c.oid =3D %u "
                          "AND c.oid =3D 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/pg=
test/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.

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company