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

On Mon, 8 Feb 2021, 14:46 Pavel Borisov <pashkin.elfe@gmail.com wrote:

Hi, hackers!

It seems that if btree index with a unique constraint is corrupted by
duplicates, amcheck now can not catch this. Reindex becomes impossible as
it throws an error but otherwise the index will let the user know that it
is corrupted, and amcheck will tell that the index is clean. So I'd like to
propose a short patch to improve amcheck for checking the unique
constraint. It will output tid's of tuples that are duplicated in the index
(i.e. more than one tid for the same index key is visible) and the user can
easily investigate and delete corresponding table entries.

0001 - is the actual patch, and
0002 - is a temporary hack for testing. It will allow inserting duplicates
in a table even if an index with the exact name "idx" has a unique
constraint (generally it is prohibited to insert). Then a new amcheck will
tell us about these duplicates. It's pity but testing can not be done
automatically, as it needs a core recompile. For testing I'd recommend a
protocol similar to the following:

- Apply patch 0002
- Set autovaccum = off in postgresql.conf

*create table tbl2 (a varchar(50), b varchar(150), c bytea, d
varchar(50));create unique index idx on tbl2(a,b);insert into tbl2 select
i::text::varchar, i::text::varchar, i::text::bytea, i::text::varchar from
generate_series(0,3) as i, generate_series(0,10000) as x;*

So we'll have a generous amount of duplicates in the table and index. Then:

*create extension amcheck;*
*select bt_index_check('idx', true);*

There will be output about each pair of duplicates: index tid's, position
in a posting list (if the index item is deduplicated) and table tid's.

WARNING: index uniqueness is violated for index "idx": Index tid=(26,6)
posting 218 and posting 219 (point to heap tid=(126,93) and tid=(126,94))
page lsn=0/1B3D420.
WARNING: index uniqueness is violated for index "idx": Index tid=(26,6)
posting 219 and posting 220 (point to heap tid=(126,94) and tid=(126,95))
page lsn=0/1B3D420.
WARNING: index uniqueness is violated for index "idx": Index tid=(26,6)
posting 220 and posting 221 (point to heap tid=(126,95) and tid=(126,96))
page lsn=0/1B3D420.
WARNING: index uniqueness is violated for index "idx": Index tid=(26,6)
posting 221 and tid=(26,7) posting 0 (point to heap tid=(126,96) and
tid=(126,97)) page lsn=0/1B3D420.
WARNING: index uniqueness is violated for index "idx": Index tid=(26,7)
posting 0 and posting 1 (point to heap tid=(126,97) and tid=(126,98)) page
lsn=0/1B3D420.
WARNING: index uniqueness is violated for index "idx": Index tid=(26,7)
posting 1 and posting 2 (point to heap tid=(126,98) and tid=(126,99)) page
lsn=0/1B3D420.

Things to notice in the test output:
- cross-page duplicates (when some of them on the one index page and the
other are on the next). (Under patch 0002 they are marked by an additional
message "*INFO: cross page equal keys"* to catch them among the other)

- If we delete table entries corresponding to some duplicates in between
the other duplicates (vacuum should be off), the message for this entry
should disappear but the other duplicates should be detected as previous.

*delete from tbl2 where ctid::text='(126,94)';*
*select bt_index_check('idx', true);*

WARNING: index uniqueness is violated for index "idx": Index tid=(26,6)
posting 218 and posting 220 (point to heap tid=(126,93) and tid=(126,95))
page lsn=0/1B3D420.
WARNING: index uniqueness is violated for index "idx": Index tid=(26,6)
posting 220 and posting 221 (point to heap tid=(126,95) and tid=(126,96))
page lsn=0/1B3D420.
WARNING: index uniqueness is violated for index "idx": Index tid=(26,6)
posting 221 and tid=(26,7) posting 0 (point to heap tid=(126,96) and
tid=(126,97)) page lsn=0/1B3D420.
WARNING: index uniqueness is violated for index "idx": Index tid=(26,7)
posting 0 and posting 1 (point to heap tid=(126,97) and tid=(126,98)) page
lsn=0/1B3D420.
WARNING: index uniqueness is violated for index "idx": Index tid=(26,7)
posting 1 and posting 2 (point to heap tid=(126,98) and tid=(126,99)) page
lsn=0/1B3D420.

Caveat: if the first entry on the next index page has a key equal to the
key on a previous page AND all heap tid's corresponding to this entry are
invisible, currently cross-page check can not detect unique constraint
violation between previous index page entry and 2nd, 3d and next current
index page entries. In this case, there would be a message that recommends
doing VACUUM to remove the invisible entries from the index and repeat the
check. (Generally, it is recommended to do vacuum before the check, but for
the testing purpose I'd recommend turning it off to check the detection of
visible-invisible-visible duplicates scenarios)

Your feedback is very much welcome, as usual.

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;

There was typo, I mean, initially"...index will NOT let the user know that
it is corrupted..."

On Feb 8, 2021, at 2:46 AM, Pavel Borisov <pashkin.elfe@gmail.com> wrote:

0002 - is a temporary hack for testing. It will allow inserting duplicates in a table even if an index with the exact name "idx" has a unique constraint (generally it is prohibited to insert). Then a new amcheck will tell us about these duplicates. It's pity but testing can not be done automatically, as it needs a core recompile. For testing I'd recommend a protocol similar to the following:

- Apply patch 0002
- Set autovaccum = off in postgresql.conf

Thanks Pavel and Anastasia for working on this!

Updating pg_catalog directly is ugly, but the following seems a simpler way to set up a regression test than having to recompile. What do you think?

CREATE TABLE junk (t text);
CREATE UNIQUE INDEX junk_idx ON junk USING btree (t);
INSERT INTO junk (t) VALUES ('fee'), ('fi'), ('fo'), ('fum');
UPDATE pg_catalog.pg_index
SET indisunique = false
WHERE indrelid = (SELECT oid FROM pg_catalog.pg_class WHERE relname = 'junk');
INSERT INTO junk (t) VALUES ('fee'), ('fi'), ('fo'), ('fum');
UPDATE pg_catalog.pg_index
SET indisunique = true
WHERE indrelid = (SELECT oid FROM pg_catalog.pg_class WHERE relname = 'junk');
SELECT * FROM junk;
t
-----
fee
fi
fo
fum
fee
fi
fo
fum
(8 rows)

\d junk
Table "public.junk"
Column | Type | Collation | Nullable | Default
--------+------+-----------+----------+---------
t | text | | |
Indexes:
"junk_idx" UNIQUE, btree (t)

\d junk_idx
Index "public.junk_idx"
Column | Type | Key? | Definition
--------+------+------+------------
t | text | yes | t
unique, btree, for table "public.junk"

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

вт, 9 февр. 2021 г. в 01:46, Mark Dilger <mark.dilger@enterprisedb.com>:

On Feb 8, 2021, at 2:46 AM, Pavel Borisov <pashkin.elfe@gmail.com>

wrote:

0002 - is a temporary hack for testing. It will allow inserting

duplicates in a table even if an index with the exact name "idx" has a
unique constraint (generally it is prohibited to insert). Then a new
amcheck will tell us about these duplicates. It's pity but testing can not
be done automatically, as it needs a core recompile. For testing I'd
recommend a protocol similar to the following:

- Apply patch 0002
- Set autovaccum = off in postgresql.conf

Thanks Pavel and Anastasia for working on this!

Updating pg_catalog directly is ugly, but the following seems a simpler
way to set up a regression test than having to recompile. What do you
think?

Very nice idea, thanks!

I've made a regression test based on it. PFA v.2 of a patch. Now it doesn't
need anything external for testing.

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;

To make tests stable I also removed lsn output under warning level. PFA v3
of a patch

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;

Hi,
Minor comment:

+ if (errflag == true)
+ ereport(ERROR,

I think 'if (errflag)' should suffice.

Cheers

On Tue, Feb 9, 2021 at 10:44 AM Pavel Borisov <pashkin.elfe@gmail.com>
wrote:

On Feb 9, 2021, at 10:43 AM, Pavel Borisov <pashkin.elfe@gmail.com> wrote:

вт, 9 февр. 2021 г. в 01:46, Mark Dilger <mark.dilger@enterprisedb.com>:

On Feb 8, 2021, at 2:46 AM, Pavel Borisov <pashkin.elfe@gmail.com> wrote:

0002 - is a temporary hack for testing. It will allow inserting duplicates in a table even if an index with the exact name "idx" has a unique constraint (generally it is prohibited to insert). Then a new amcheck will tell us about these duplicates. It's pity but testing can not be done automatically, as it needs a core recompile. For testing I'd recommend a protocol similar to the following:

- Apply patch 0002
- Set autovaccum = off in postgresql.conf

Thanks Pavel and Anastasia for working on this!

Updating pg_catalog directly is ugly, but the following seems a simpler way to set up a regression test than having to recompile. What do you think?

Very nice idea, thanks!
I've made a regression test based on it. PFA v.2 of a patch. Now it doesn't need anything external for testing.

If bt_index_check() and bt_index_parent_check() are to have this functionality, shouldn't there be an option controlling it much as the option (heapallindexed boolean) controls checking whether all entries in the heap are indexed in the btree? It seems inconsistent to have an option to avoid checking the heap for that, but not for this. Alternately, this might even be better coded as its own function, named something like bt_unique_index_check() perhaps. I hope Peter might advise?

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.

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

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.

May I ask you to ensure you used v3 of a patch to check? I've made tests
portable in v3, probably, you've checked not the last version.

Thanks for your attention to the patch
--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;

On Mar 1, 2021, at 12:05 PM, Pavel Borisov <pashkin.elfe@gmail.com> 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.
May I ask you to ensure you used v3 of a patch to check? I've made tests portable in v3, probably, you've checked not the last version.

Yes, my review was of v2. Updating to v3, I see that the test passes on my laptop. It still looks brittle to have all the tid values in the test output, but it does pass.

Thanks for your attention to the patch

Thanks for the patch!

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

If bt_index_check() and bt_index_parent_check() are to have this
functionality, shouldn't there be an option controlling it much as the
option (heapallindexed boolean) controls checking whether all entries in
the heap are indexed in the btree? It seems inconsistent to have an option
to avoid checking the heap for that, but not for this. Alternately, this
might even be better coded as its own function, named something like
bt_unique_index_check() perhaps. I hope Peter might advise?

As for heap checking, my reasoning was that we can not check whether a
unique constraint violated by the index, without checking heap tuple
visibility. I.e. we can have many identical index entries without
uniqueness violated if only one of them corresponds to a visible heap
tuple. So heap checking included in my patch is _necessary_ for unique
constraint checking, it should not have an option to be disabled,
otherwise, the only answer we can get is that unique constraint MAY be
violated and may not be, which is quite useless. If you meant something
different, please elaborate.

I can try to rewrite unique constraint checking to be done after all others
check but I suppose it's the performance considerations are that made
previous amcheck routines to do many checks simultaneously. I tried to
stick to this practice. It's also not so elegant to duplicate much code to
make uniqueness checks independently and the resulting patch will be much
bigger and harder to review.

Anyway, your and Peter's further considerations are always welcome.

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;

Mark Dilger <mark.dilger@enterprisedb.com> writes:

Yes, my review was of v2. Updating to v3, I see that the test passes on my laptop. It still looks brittle to have all the tid values in the test output, but it does pass.

Hm, anyone tried it on 32-bit hardware?

regards, tom lane

On Mar 1, 2021, at 12:23 PM, Pavel Borisov <pashkin.elfe@gmail.com> wrote:

If bt_index_check() and bt_index_parent_check() are to have this functionality, shouldn't there be an option controlling it much as the option (heapallindexed boolean) controls checking whether all entries in the heap are indexed in the btree? It seems inconsistent to have an option to avoid checking the heap for that, but not for this. Alternately, this might even be better coded as its own function, named something like bt_unique_index_check() perhaps. I hope Peter might advise?

As for heap checking, my reasoning was that we can not check whether a unique constraint violated by the index, without checking heap tuple visibility. I.e. we can have many identical index entries without uniqueness violated if only one of them corresponds to a visible heap tuple. So heap checking included in my patch is _necessary_ for unique constraint checking, it should not have an option to be disabled, otherwise, the only answer we can get is that unique constraint MAY be violated and may not be, which is quite useless. If you meant something different, please elaborate.

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'm not against running uniqueness checks on unique indexes. It seems fairly normal that a user would want that. Perhaps the option should default to 'true' if unspecified? But having no option at all seems to run contrary to how the other functionality is structured.

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

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 <http://www.postgrespro.com&gt;

On Mon, Mar 1, 2021 at 11:22 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

If bt_index_check() and bt_index_parent_check() are to have this functionality, shouldn't there be an option controlling it much as the option (heapallindexed boolean) controls checking whether all entries in the heap are indexed in the btree? It seems inconsistent to have an option to avoid checking the heap for that, but not for this.

I agree. Actually, it should probably use the same snapshot as the
heapallindexed=true case. So either only perform unique constraint
verification when that option is used, or invent a new option that
will still share the snapshot used by heapallindexed=true (when the
options are combined).

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

--
Peter Geoghegan

On Mon, Feb 8, 2021 at 2:46 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote:

Caveat: if the first entry on the next index page has a key equal to the key on a previous page AND all heap tid's corresponding to this entry are invisible, currently cross-page check can not detect unique constraint violation between previous index page entry and 2nd, 3d and next current index page entries. In this case, there would be a message that recommends doing VACUUM to remove the invisible entries from the index and repeat the check. (Generally, it is recommended to do vacuum before the check, but for the testing purpose I'd recommend turning it off to check the detection of visible-invisible-visible duplicates scenarios)

It's rather unlikely that equal values in a unique index will end up
on different leaf pages. It's really rare, in fact. This following
comment block from nbtinsert.c (which appears right before we call
_bt_check_unique()) explains why this is:

* It might be necessary to check a page to the right in _bt_check_unique,
* though that should be very rare. In practice the first page the value ...

You're going to have to "couple" buffer locks in the style of
_bt_check_unique() (as well as keeping a buffer lock on "the first
leaf page a duplicate might be on" throughout) if you need the test to
work reliably. But why bother with that? The tool doesn't have to be
100% perfect at detecting corruption (nothing can be), and it's rather
unlikely that it will matter for this test. A simple test that doesn't
handle cross-page duplicates is still going to be very effective.

I don't think that it's acceptable for your new check to raise a
WARNING instead of an ERROR. I especially don't like that the new
unique checking functionality merely warns that the index *might* be
corrupt. False positives are always unacceptable within amcheck, and I
think that this is a false positive.

--
Peter Geoghegan

You're going to have to "couple" buffer locks in the style of
_bt_check_unique() (as well as keeping a buffer lock on "the first
leaf page a duplicate might be on" throughout) if you need the test to
work reliably. But why bother with that? The tool doesn't have to be
100% perfect at detecting corruption (nothing can be), and it's rather
unlikely that it will matter for this test. A simple test that doesn't
handle cross-page duplicates is still going to be very effective.

Indeed at first, I did the test which doesn't bother checking duplicates
cross-page which I considered very rare, but then a customer sent me his
corrupted index where I found this rare thing which was not detectable by
amcheck and he was puzzled with the issue. Even rare inconsistencies can
appear when people handle huge amounts of data. So I did an update that
handles a wider class of errors. I don't suppose that cross page unique
check is expensive as it uses same things that are already used in amcheck
for cross-page checks.

Is it suitable if I omit suspected duplicates message in the very-very rare
case amcheck can not detect but leave cross-page checks?

I don't think that it's acceptable for your new check to raise a
WARNING instead of an ERROR.

It is not instead of an ERROR. If at least one violation is detected,
amcheck will output the final ERROR message. The purpose is not to stop
checking at the first violation. But I can make them reported in a current
amcheck style if it is necessary.

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;

On Mar 2, 2021, at 6:08 AM, Pavel Borisov <pashkin.elfe@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 <pg@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 <pg@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

On 3/15/21 11:11 AM, Mark Dilger wrote:

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

This patch appears to need some work and has not been updated in several
weeks, so marking Returned with Feedback.

Please submit to the next CF when you have a new patch.

Regards,
--
-David
david@pgmasters.net

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&gt;

On Dec 20, 2021, at 7:37 AM, Pavel Borisov <pashkin.elfe@gmail.com> wrote:

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 := c.oid, heapallindexed := %s, rootdescend := %s)"
+                         "index := c.oid, heapallindexed := %s, rootdescend := %s, "
+                         "checkunique := %s)"
                          "\nFROM pg_catalog.pg_class c, pg_catalog.pg_index i "
                          "WHERE c.oid = %u "
                          "AND c.oid = 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/pgtest/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.

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