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

> On Mar 2, 2021, at 6:08 AM, Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com> wrote:
>
> I completely agree that checking uniqueness requires looking at the heap, but I don't agree that every caller of bt_index_check on an index wants that particular check to be performed. There are multiple ways in which an index might be corrupt, and Peter wrote the code to only check some of them by default, with options to expand the checks to other things. This is why heapallindexed is optional. If you don't want to pay the price of checking all entries in the heap against the btree, you don't have to.
>
> I've got the idea and revised the patch accordingly. Thanks!
> Pfa v4 of a patch. I've added an optional argument to allow uniqueness checks for the unique indexes.
> Also, I added a test variant to make them work on 32-bit systems. Unfortunately, converting the regression test to TAP would be a pain for me. Hope it can be used now as a 2-variant regression test for 32 and 64 bit systems.
>
> Thank you for your consideration!
>
> --
> Best regards,
> Pavel Borisov
>
> Postgres Professional: http://postgrespro.com
> <v4-0001-Make-amcheck-checking-UNIQUE-constraint-for-btree.patch>

Looking over v4, here are my review comments...

I created the file contrib/amcheck/amcheck--1.2--1.3.sql during the v14 development cycle, so that is not released yet. If your patch goes out in v14, does it need to create an amcheck--1.3--1.4.sql, or could you fit your changes into the 1.2--1.3.sql file? (Does the project have a convention governing this?) This is purely a question. I'm not advising you to change anything here.

You need to update doc/src/sgml/amcheck.sgml to reflect the changes you made to the bt_index_check and bt_index_parent_check functions.

You need to update the recently committed src/bin/pg_amcheck project to include --checkunique as an option. This client application already has flags for heapallindexed and rootdescend. I can help with that if it isn't obvious what needs to be done. Note that pg_amcheck/t contains TAP tests that exercise the options, so you'll need to extend code coverage to include this new option.

> On Mar 2, 2021, at 7:10 PM, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
>
> I don't think that it's acceptable for your new check to raise a
> WARNING instead of an ERROR.

You already responded to Peter, and I can see that after raising WARNINGs about an index, the code raises an ERROR. That is different from behavior that pg_amcheck currently expects from contrib/amcheck functions. It will be interesting to see if that makes integration harder.

> On Mar 2, 2021, at 6:54 PM, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
>
>> The regression test you provided is not portable. I am getting lots of errors due to differing output of the form "page lsn=0/4DAD7E0". You might turn this into a TAP test and use a regular expression to check the output.
>
> I would test this using a custom opclass that does simple fault
> injection. For example, an opclass that indexes integers, but can be
> configured to dynamically make 0 values equal or unequal to each
> other. That's more representative of real-world problems.
>
> You "break the warranty" by updating pg_index, even compared to
> updating other system catalogs. In particular, you break the
> "indcheckxmin wait -- wait for xmin to be old before using index"
> stuff in get_relation_info(). So it seems worse than updating
> pg_attribute, for example (which is something that the tests do
> already).

Take a look at src/bin/pg_amcheck/t/005_opclass_damage.pl for an example of changing an opclass to test btree index breakage.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company