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

Aleksander Alekseev <aleksander@timescale.com>

From: Aleksander Alekseev <aleksander@timescale.com>

To: Postgres hackers <pgsql-hackers@lists.postgresql.org>

Cc: Pavel Borisov <pashkin.elfe@gmail.com>, Maxim Orlov <orlovmg@gmail.com>, lubennikovaav@gmail.com, Mark Dilger <mark.dilger@enterprisedb.com>, Peter Geoghegan <pg@bowt.ie>

Date: 2022-07-20T14:15:34Z

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 →

amcheck: Optimize speed of checking for unique constraint violation
- cdd6ab9d1f53 18.0 landed
amcheck: Report an error when the next page to a leaf is not a leaf
- 97e5b0026fc2 17.0 landed
amcheck: Don't load the right sibling page into BtreeCheckState
- 0b5c16124811 17.0 landed
amcheck: Refactoring the storage of the last visible entry
- 532d94fec32a 17.0 landed
Teach contrib/amcheck to check the unique constraint violation
- 5ae2087202af 17.0 landed
Add macros in hash and btree AMs to get the special area of their pages
- d16773cdc862 15.0 cited

Attachments

v14-0001-Add-option-for-amcheck-and-pg_amcheck-to-check-u.patch (application/octet-stream) patch v14-0001

Hi Pavel,

> Rebased. PFA v13.
> Your reviews are very much welcome!

I noticed that this patch is in "Needs Review" state and it has been
stuck for some time now, so I decided to take a look.

```
+SELECT bt_index_parent_check('bttest_a_idx', true, true, true);
+SELECT bt_index_parent_check('bttest_b_idx', true, false, true);
``

1. This "true, false, true" sequence is difficult to read. I suggest
we use named arguments here.

2. I believe there are some minor issues with the comments. E.g. instead of:

- First key on next page is same
- Make values 768 and 769 looks equal

I would write:

- The first key on the next page is the same
- Make values 768 and 769 look equal

There are many little errors like these.

```
+# Copyright (c) 2021, PostgreSQL Global Development Group
```

3. Oh no. The copyright has expired!

```
+      <literal>true</literal>.  When <parameter>checkunique</parameter>
+      is <literal>true</literal> <function>bt_index_check</function> will
```

4. This piece of documentation was copy-pasted between two functions
without change of the function name.

Other than that to me the patch looks in pretty good shape. Here is
v14 where I fixed my own nitpicks, with the permission of Pavel given
offlist.

If no one sees any other defects I'm going to change the status of the
patch to "Ready to Committer" in a short time.

--
Best regards,
Aleksander Alekseev