Horton

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

Pavel Borisov <pashkin.elfe@gmail.com>

View thread
From: Pavel Borisov <pashkin.elfe@gmail.com>
To: David Steele <david@pgmasters.net>
Cc: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>, Postgres hackers <pgsql-hackers@lists.postgresql.org>, Peter Geoghegan <pg@bowt.ie>, Mark Dilger <mark.dilger@enterprisedb.com>, Maxim Orlov <m.orlov@postgrespro.ru>
Date: 2021-12-20T15:37:35Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. amcheck: Optimize speed of checking for unique constraint violation

  2. amcheck: Report an error when the next page to a leaf is not a leaf

  3. amcheck: Don't load the right sibling page into BtreeCheckState

  4. amcheck: Refactoring the storage of the last visible entry

  5. Teach contrib/amcheck to check the unique constraint violation

  6. Add macros in hash and btree AMs to get the special area of their pages

Attachments

>
> >> 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...
>

Mark and Peter, big thanks for your ideas!

I had little time to work on this feature until recently, but finally, I've
elaborated v5 patch (PFA)
It contains the following improvements, most of which are based on your
consideration:

- Amcheck tests are reworked into TAP-tests with "break the warranty" by
comparison function changes in the opclass instead of pg_index update.
Mark, again thanks for the sample!
- Added new --checkunique option into pg_amcheck.
- Added documentation and tests for amcheck and pg_amcheck new functions.
- Results are output at ERROR log level like it is done in the other
amcheck tests.
- Rare case of inability to check due to the first entry on a leaf page
being both: (1) equal to the last one on the previous page and (2) deleted
in the heap, is demoted to DEBUG1 log level. In this, I folowed Peter's
consideration that amcheck should do its best to check, but can not always
verify everything. The case is expected to be extremely rare.
- Made pages connectivity based on btpo_next (corrected a bug in the code,
I found meanwhile)
- If snapshot is already taken in heapallindexed case, reuse it for unique
constraint check.

The patch is pgindented and rebased on the current PG master code.
I'd like  to re-attach the patch v5 to the upcoming CF if you don't mind.

I also want to add that some customers face index uniqueness
violations more often than I've expected, and I hope this check could also
help some other PostgreSQL customers.

Your further considerations are welcome as always!
-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>