> On Mar 2, 2021, at 6:08 AM, Pavel Borisov <pashkin.e...@gmail.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 <p...@bowt.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 <p...@bowt.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
