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

From: Alexander Korotkov <[email-masked]>
Date: Fri, 10 May 2024 22:05:01 +0300

On Fri, May 10, 2024 at 8:35 PM Mark Dilger
<[email-masked]> wrote:
> > On May 10, 2024, at 5:10 AM, Pavel Borisov <[email-masked]> wrote:
> > On Fri, 10 May 2024 at 12:39, Alexander Korotkov <[email-masked]> wrote:
> > On Fri, May 10, 2024 at 3:43 AM Tom Lane <[email-masked]> wrote:
> > > Alexander Korotkov <[email-masked]> writes:
> > > > The revised patchset is attached.  I applied cosmetical changes.  I'm
> > > > going to push it if no objections.
> > >
> > > Is this really suitable material to be pushing post-feature-freeze?
> > > It doesn't look like it's fixing any new-in-v17 issues.
> >
> > These are code improvements to the 5ae2087202, which answer critics in
> > the thread.  0001 comprises an optimization, but it's rather small and
> > simple.  0002 and 0003 contain refactoring.  0004 contains better
> > error reporting.  For me this looks like pretty similar to what others
> > commit post-FF, isn't it?
> > I've re-checked patches v2. Differences from v1 are in improving naming/pgindent's/commit messages.
> > In 0002 order of variables in struct BtreeLastVisibleEntry changed. This doesn't change code behavior.
> >
> > Patch v2-0003 doesn't contain credits and a discussion link. All other patches do.
> >
> > Overall, patches contain small performance optimization (0001), code refactoring and error reporting changes. IMO they could be pushed post-FF.
>
> v2-0001's commit message itself says, "This commit implements skipping keys". I take no position on the correctness or value of the improvement, but it seems out of scope post feature freeze.  The patch seems to postpone uniqueness checking until later in the scan than what the prior version did, and that kind of change could require more analysis than we have time for at this point in the release cycle.

Formally this could be classified as algorithmic change and probably
should be postponed to the next release.  But that's quite local
optimization, which just postpones a function call within the same
iteration of loop.  And the effect is huge.  Probably we could allow
this post-FF in the sake of quality release, given it's very local
change with a huge effect.

> v2-0002 does appear to just be refactoring.  I don't care for a small portion of that patch, but I doubt it violates the post feature freeze rules.  In particular:
>
>   +   BtreeLastVisibleEntry lVis = {InvalidBlockNumber, InvalidOffsetNumber, -1, NULL};

I don't understand what is the problem with this line and post feature
freeze rules.  Please, explain it more.

> v2-0003 may be an improvement in some way, but it compounds some preexisting confusion also.  There is already a member of the BtreeCheckState called "target" and a memory context in that struct called "targetcontext".  That context is used to allocate pages "state->target", "rightpage", "child" and "page", but not "metapage".  Perhaps "targetcontext" is a poor choice of name?  "notmetacontext" is a terrible name, but closer to describing the purpose of the memory context.  Care to propose something sensible?
>
> Prior to applying v2-0003, the rightpage was stored in state->target, and continued to be in state->target later when entering
>
>         /*
>          * * Downlink check *
>          *
>          * Additional check of child items iff this is an internal page and
>          * caller holds a ShareLock.  This happens for every downlink (item)
>          * in target excluding the negative-infinity downlink (again, this is
>          * because it has no useful value to compare).
>          */
>         if (!P_ISLEAF(topaque) && state->readonly)
>             bt_child_check(state, skey, offset);
>
> and thereafter.  Now, the rightpage of state->target is created, checked, and free'd, and then the old state->target gets processed in the downlink check and thereafter.  This is either introducing a bug, or fixing one, but the commit message is totally ambiguous about whether this is a bugfix or a code cleanup or something else?  I think this kind of patch should have a super clear commit message about what it thinks it is doing.

The only bt_target_page_check() caller is
bt_check_level_from_leftmost(), which overrides state->target in the
next iteration anyway.  I think the patch is just refactoring to
eliminate the confusion pointer by Peter Geoghegan upthread.

0002 and 0003 don't address any bugs, but It would be very nice to
accept them, because it would simplify future backpatching in this
area.

> v2-0004 guards against a real threat, and is reasonable post feature freeze

Ok.

------
Regards,
Alexander Korotkov
Supabase

---

From: Mark Dilger <[email-masked]>
Date: Fri, 10 May 2024 18:12:57 -0700

> On May 10, 2024, at 12:05 PM, Alexander Korotkov <[email-masked]> wrote:
> 

I find your argument unconvincing.

After bt_target_page_check() returns at line 919, and before bt_check_level_from_leftmost() overrides state->target in the next iteration, bt_check_level_from_leftmost() conditionally fetches an item from the page referenced by state->target.  See line 963.

I'm left with four possibilities:

1)  bt_target_page_check() never gets to the code that uses "rightpage" rather than "state->target" in the same iteration where bt_check_level_from_leftmost() conditionally fetches an item from state->target, so the change you're making doesn't matter.

2)  The code prior to v2-0003 was wrong, having changed state->target in an inappropriate way, causing the wrong thing to happen at what is now line 963.  The patch fixes the bug, because state->target no longer gets overwritten where you are now using "rightpage" for the value.

3)  The code used to work, having set up state->target correctly in the place where you are now using "rightpage", but v2-0003 has broken that. 

4)  It's been broken all along and your patch just changes from wrong to wrong.

If you believe (1) is true, then I'm complaining that you are relying far to much on action at a distance, and that you are not documenting it.  Even with documentation of this interrelationship, I'd be unhappy with how brittle the code is.  I cannot easily discern that the two don't ever happen in the same iteration, and I'm not at all convinced one way or the other.  I tried to set up some Asserts about that, but none of the test cases actually reach the new code, so adding Asserts doesn't help to investigate the question.

If (2) is true, then I'm complaining that the commit message doesn't mention the fact that this is a bug fix.  Bug fixes should be clearly documented as such, otherwise future work might assume the commit can be reverted with only stylistic consequences.

If (3) is true, then I'm complaining that the patch is flat busted.

If (4) is true, then maybe we should revert the entire feature, or have a discussion of mitigation efforts that are needed.

Regardless of which of 1..4 you pick, I think it could all do with more regression test coverage.

For reference, I said something similar earlier today in another email to this thread:

This patch introduces a change that stores a new page into variable "rightpage" rather than overwriting "state->target", which the old implementation most certainly did.  That means that after returning from bt_target_page_check() into the calling function bt_check_level_from_leftmost() the value in state->target is not what it would have been prior to this patch.  Now, that'd be irrelevant if nobody goes on to consult that value, but just 44 lines further down in bt_check_level_from_leftmost() state->target is clearly used.  So the behavior at that point is changing between the old and new versions of the code, and I think I'm within reason to ask if it was wrong before the patch, wrong after the patch, or something else?  Is this a bug being introduced, being fixed, or ... ?

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

---

From: Tom Lane <[email-masked]>
Date: Fri, 10 May 2024 21:38:57 -0400

> Regardless of which of 1..4 you pick, I think it could all do with more regression test coverage.

Indeed.  If we have no regression tests that reach this code, it's
folly to touch it at all, but most especially so post-feature-freeze.

I think the *first* order of business ought to be to create some
test cases that reach this area.  Perhaps they'll be too expensive
to incorporate in our regular regression tests, but we could still
use them to investigate Mark's concerns.

			regards, tom lane

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 13 May 2024 00:23:42 +0300

> This patch introduces a change that stores a new page into variable "rightpage" rather than overwriting "state->target", which the old implementation most certainly did.  That means that after retu…

Thank you for your analysis.  I'm inclined to believe in 2, but not
yet completely sure.  It's really pity that our tests don't cover
this.  I'm investigating this area.

------
Regards,
Alexander Korotkov

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 13 May 2024 04:42:20 +0300

> this.  I'm investigating this area.

It seems that I got to the bottom of this.  Changing
BtreeCheckState.target for a cross-page unique constraint check is
wrong, but that happens only for leaf pages.  After that
BtreeCheckState.target is only used for setting the low key.  The low
key is only used for non-leaf pages.  So, that didn't lead to any
visible bug.  I've revised the commit message to reflect this.

So, the picture for the patches is the following now.
0001 – optimization, but rather simple and giving huge effect
0002 – refactoring
0003 – fix for the bug
0004 – better error reporting

------
Regards,
Alexander Korotkov

---

From: Pavel Borisov <[email-masked]>
Date: Mon, 13 May 2024 15:55:11 +0400

Hi, Alexander!

On Mon, 13 May 2024 at 05:42, Alexander Korotkov <[email-masked]>
wrote:

> > > After bt_target_page_check() returns at line 919, and before
> bt_check_level_from_leftmost() overrides state->target in the next
> iteration, bt_check_level_from_leftmost() conditionally fetches an item
> from the page referenced by state->target.  See line 963.
> > >
> > > 1)  bt_target_page_check() never gets to the code that uses
> "rightpage" rather than "state->target" in the same iteration where
> bt_check_level_from_leftmost() conditionally fetches an item from
> state->target, so the change you're making doesn't matter.
> > >
> > > 2)  The code prior to v2-0003 was wrong, having changed state->target
> in an inappropriate way, causing the wrong thing to happen at what is now
> line 963.  The patch fixes the bug, because state->target no longer gets
> overwritten where you are now using "rightpage" for the value.
> > >
> > > 3)  The code used to work, having set up state->target correctly in
> the place where you are now using "rightpage", but v2-0003 has broken that.
> > >
> > > If you believe (1) is true, then I'm complaining that you are relying
> far to much on action at a distance, and that you are not documenting it.
> Even with documentation of this interrelationship, I'd be unhappy with how
> brittle the code is.  I cannot easily discern that the two don't ever
> happen in the same iteration, and I'm not at all convinced one way or the
> other.  I tried to set up some Asserts about that, but none of the test
> cases actually reach the new code, so adding Asserts doesn't help to
> investigate the question.
> > >
> > > If (2) is true, then I'm complaining that the commit message doesn't
> mention the fact that this is a bug fix.  Bug fixes should be clearly
> documented as such, otherwise future work might assume the commit can be
> reverted with only stylistic consequences.
> > >
> > > If (4) is true, then maybe we should revert the entire feature, or
> have a discussion of mitigation efforts that are needed.
> > >
> > > This patch introduces a change that stores a new page into variable
> "rightpage" rather than overwriting "state->target", which the old
> implementation most certainly did.  That means that after returning from
> bt_target_page_check() into the calling function
> bt_check_level_from_leftmost() the value in state->target is not what it
> would have been prior to this patch.  Now, that'd be irrelevant if nobody
> goes on to consult that value, but just 44 lines further down in
> bt_check_level_from_leftmost() state->target is clearly used.  So the
> behavior at that point is changing between the old and new versions of the
> code, and I think I'm within reason to ask if it was wrong before the
> patch, wrong after the patch, or something else?  Is this a bug being
> introduced, being fixed, or ... ?
> >

I agree with your analysis regarding state->target:
- when the unique check is on, state->target was reassigned only for the
leaf pages (under P_ISLEAF(topaque) in bt_target_page_check).
- in this level (leaf) in bt_check_level_from_leftmost() this value of
state->target was used to get state->lowkey. Then it was reset (in the next
iteration of do loop in in bt_check_level_from_leftmost()
- state->lowkey lives until the end of pages level (leaf) iteration cycle.
Then, low-key is reset (state->lowkey = NULL in the end of
 bt_check_level_from_leftmost())
- state->lowkey is used only in bt_child_check/bt_child_highkey_check. Both
are called only from non-leaf pages iteration cycles (under
P_ISLEAF(topaque))
- Also there is a check (rightblock_number != P_NONE) in before getting
rightpage into state->target in bt_target_page_check() that ensures us that
rightpage indeed exists and getting this (unused) lowkey in
bt_check_level_from_leftmost will not invoke any page reading errors.

I'm pretty sure that there was no bug in this, not just the bug was hidden.

Indeed re-assigning state->target in leaf page iteration for cross-page
unique check was not beautiful, and Peter pointed out this. In my opinion
the patch 0003 is a pure code refactoring.

As for the cross-page check regression/TAP testing, this test had problems
since the btree page layout is not fixed (especially it's different on
32-bit arch). I had a variant for testing cross-page check when the test
was yet regression one upthread for both 32/64 bit architectures. I
remember it was decided not to include it due to complications and low
impact for testing the corner case of very rare cross-page duplicates.
(There were also suggestions to drop cross-page duplicates check at all,
which I didn't agree 2 years ago, but still it can make sense)

Separately, I propose to avoid getting state->lowkey for leaf pages at all
as it's unused. PFA is a simple patch for this. (I don't add it to the
current patch set as I believe it has nothing to do with UNIQUE constraint
check, rather it improves the previous btree amcheck code)

Best regards,
Pavel Borisov,
Supabase

---

From: Pavel Borisov <[email-masked]>
Date: Mon, 13 May 2024 16:19:54 +0400

On Mon, 13 May 2024 at 15:55, Pavel Borisov <[email-masked]> wrote:

> - state->lowkey is used only in bt_child_check/bt_child_highkey_check.
> Both are called only from non-leaf pages iteration cycles (under
> P_ISLEAF(topaque))

A correction of a typo in previous message:
non-leaf pages iteration cycles (under !P_ISLEAF(topaque)) -> non-leaf
pages iteration cycles (under !P_ISLEAF(topaque))

---

From: Pavel Borisov <[email-masked]>
Date: Mon, 13 May 2024 16:20:25 +0400

A correction of a typo in previous message:
non-leaf pages iteration cycles (under P_ISLEAF(topaque)) -> non-leaf pages
iteration cycles (under !P_ISLEAF(topaque))

On Mon, 13 May 2024 at 16:19, Pavel Borisov <[email-masked]> wrote:

>>> > > 2)  The code prior to v2-0003 was wrong, having changed
>>> state->target in an inappropriate way, causing the wrong thing to happen at
>>> what is now line 963.  The patch fixes the bug, because state->target no
>>> longer gets overwritten where you are now using "rightpage" for the value.
>>> > >
>>> > > If you believe (1) is true, then I'm complaining that you are
>>> relying far to much on action at a distance, and that you are not
>>> documenting it.  Even with documentation of this interrelationship, I'd be
>>> unhappy with how brittle the code is.  I cannot easily discern that the two
>>> don't ever happen in the same iteration, and I'm not at all convinced one
>>> way or the other.  I tried to set up some Asserts about that, but none of
>>> the test cases actually reach the new code, so adding Asserts doesn't help
>>> to investigate the question.
>>> > >
>> - state->lowkey lives until the end of pages level (leaf) iteration
>> cycle. Then, low-key is reset (state->lowkey = NULL in the end of
>> As for the cross-page check regression/TAP testing, this test had
>> problems since the btree page layout is not fixed (especially it's
>> different on 32-bit arch). I had a variant for testing cross-page check
>> when the test was yet regression one upthread for both 32/64 bit
>> architectures. I remember it was decided not to include it due to
>> complications and low impact for testing the corner case of very rare
>> cross-page duplicates. (There were also suggestions to drop cross-page
>> duplicates check at all, which I didn't agree 2 years ago, but still it can
>> make sense)
>>
>> Separately, I propose to avoid getting state->lowkey for leaf pages at
>> all as it's unused. PFA is a simple patch for this. (I don't add it to the

---

From: Alexander Korotkov <[email-masked]>
Date: Fri, 17 May 2024 13:11:32 +0300

> 0004 – better error reporting

I think the thread contains enough motivation on why 0002, 0003 and
0004 are material for post-FF.  They are fixes and refactoring for
new-in-v17 feature.  I'm going to push them if no objections.

Regarding 0001, I'd like to ask Tom and Mark if they find convincing
that given that optimization is small, simple and giving huge effect,
it could be pushed post-FF?  Otherwise, this could wait for v18.

------
Regards,
Alexander Korotkov
Supabase

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 17 May 2024 15:09:04 +0400

Hi, Alexander!

> it could be pushed post-FF?  Otherwise, this could wait for v18.

In my view, patches 0002-0004 are worth pushing.
0001 is ready in my view. But I see no problem pushing it into v18
regarding that this optimization could be not eligible for post-FF. I don't
know the criteria for this just let's be safe about it.

Regards,
Pavel Borisov

---

From: Mark Dilger <[email-masked]>
Date: Fri, 17 May 2024 10:41:19 -0700

> On May 17, 2024, at 3:11 AM, Alexander Korotkov <[email-masked]> wrote:
> 

I won't pretend to be part of the Release Management Team.  Perhaps Tom wishes to respond.

I wrote a TAP test to check the uniqueness checker.  bt_index_check() sometimes fails to detect a corruption.  This is true both before and after applying v3-0001.  The bt_index_parent_check() seems to always detect the corruption created by the TAP test.  Likewise, this is true both before and after applying v3-0001.

The documentation in https://www.postgresql.org/docs/devel/amcheck.html#AMCHECK-FUNCTIONS is ambiguous:

"bt_index_check does not verify invariants that span child/parent relationships, but will verify the presence of all heap tuples as index tuples within the index when heapallindexed is true. When checkunique is true bt_index_check will check that no more than one among duplicate entries in unique index is visible. When a routine, lightweight test for corruption is required in a live production environment, using bt_index_check often provides the best trade-off between thoroughness of verification and limiting the impact on application performance and availability."

The second sentence, "When checkunique is true bt_index_check will check that no more than one among duplicate entries in unique index is visible." is not strictly true, as it won't check if the violation spans a page boundary.  That's implied by the surrounding sentences, but I'm not sure a reader can be trusted to know which way to interpret how "checkunique" works.  Clarification is needed.

The attached TAP test is not intended for commit.  I am only including it here because you might want to use the TAP test as a starting point for creating and testing for new kinds of corruption.  Beware the test intentionally includes an infinite loop, which is helpful for a developer examining the code, but not at all appropriate otherwise.  It loads all blocks of the index into memory each loop, which could be made more efficient if we wanted this to be part of the core codebase.  I just threw it together this morning.  It's not polished, documented, checked for portability, or otherwise production quality.

—
Mark Dilger

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 17 May 2024 22:51:38 +0400

Hi, Mark!

> The documentation in
> https://www.postgresql.org/docs/devel/amcheck.html#AMCHECK-FUNCTIONS is
> ambiguous:
>
> "bt_index_check does not verify invariants that span child/parent
> relationships, but will verify the presence of all heap tuples as index
> tuples within the index when heapallindexed is true. When checkunique is
> true bt_index_check will check that no more than one among duplicate
> entries in unique index is visible. When a routine, lightweight test for
> corruption is required in a live production environment, using
> bt_index_check often provides the best trade-off between thoroughness of
> verification and limiting the impact on application performance and
> availability."
>
> The second sentence, "When checkunique is true bt_index_check will check
> that no more than one among duplicate entries in unique index is visible."
> is not strictly true, as it won't check if the violation spans a page
> boundary.
>
Amcheck with checkunique option does check uniqueness violation between
pages. But it doesn't warranty detection of cross page uniqueness
violations in extremely rare cases when the first equal index entry on the
next page corresponds to tuple that is not visible (e.g. dead). In this, I
followed the Peter's notion [1] that checking across a number of dead equal
entries that could theoretically span even across many pages is an
unneeded code complication and amcheck is not a tool that provides any
warranty when checking an index.

I'm not against docs modification in any way that clarifies its exact usage
and limitations.

Kind regards,
Pavel Borisov

[1]
https://www.postgresql.org/message-id/CAH2-Wz%3DttG__BTZ-r5ccopBRb5evjg%3DzsF_o_3C5h4zRBA_LjQ%40mail.gmail.com

---

From: Mark Dilger <[email-masked]>
Date: Fri, 17 May 2024 12:10:10 -0700

> On May 17, 2024, at 11:51 AM, Pavel Borisov <[email-masked]> wrote:
> 
> Amcheck with checkunique option does check uniqueness violation between pages. But it doesn't warranty detection of cross page uniqueness violations in extremely rare cases when the first equal index entry on the next page corresponds to tuple that is not visible (e.g. dead). In this, I followed the Peter's notion [1] that checking across a number of dead equal entries that could theoretically span even across many pages is an unneeded code complication and amcheck is not a tool that provides any warranty when checking an index.

This confuses me a bit.  The regression test creates a table and index but never performs any DELETE nor any UPDATE operations, so none of the index entries should be dead.  If I am understanding you correct, I'd be forced to conclude that the uniqueness checking code is broken.  Can you take a look?

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Fri, 17 May 2024 12:42:26 -0700

> On May 17, 2024, at 12:10 PM, Mark Dilger <[email-masked]> wrote:
> 

On further review, the test was not anticipating the error message "high key invariant violated for index".  That wasn't seen in calls to bt_index_parent_check(), but appears as one of the errors from bt_index_check().  I am rerunning the test now....

—
Mark Dilger

---

From: Pavel Borisov <[email-masked]>
Date: Fri, 17 May 2024 23:42:42 +0400

Hi, Mark!

On Fri, 17 May 2024 at 23:10, Mark Dilger <[email-masked]>
wrote:

> entries that could theoretically span even across many pages is an unneeded
> code complication and amcheck is not a tool that provides any warranty when
> checking an index.
>
> This confuses me a bit.  The regression test creates a table and index but
> never performs any DELETE nor any UPDATE operations, so none of the index
> entries should be dead.  If I am understanding you correct, I'd be forced
> to conclude that the uniqueness checking code is broken.  Can you take a
> look?
>
At the first glance it's not clear to me:
- why your test creates cross-page unique constraint violations?
- how do you know they are not detected?

---

From: Peter Geoghegan <[email-masked]>
Date: Fri, 17 May 2024 16:00:55 -0400

> On further review, the test was not anticipating the error message "high key invariant violated for index".  That wasn't seen in calls to bt_index_parent_check(), but appears as one of the errors f…

Many different parts of the B-Tree code will fight against allowing
duplicates of the same value to span multiple leaf pages -- this is
especially true for unique indexes. For example, nbtsplitloc.c has a
variety of strategies that will prevent choosing a split point that
necessitates including a distinguishing heap TID in the new high key.
In other words, nbtsplitloc.c is very aggressive about picking a split
point between (rather than within) groups of duplicates.

Of course it's still *possible* for a unique index to have multiple
leaf pages containing the same individual value. The regression tests
do have coverage for certain relevant code paths (e.g., there is
coverage for code paths only hit when _bt_check_unique has to go to
the page to the right). This is only the case because I went out of my
way to make sure of it, by adding tests that allow a huge number of
version duplicates to accumulate within a unique index. (The "move
right" _bt_check_unique branches had zero test coverage for a year or
two.)

Just how important it is that amcheck covers cases where the version
duplicates span multiple leaf pages is of course debatable -- it's
always better to be more thorough, when practical. But it's certainly
something that needs to be assessed based on the merits.

--
Peter Geoghegan

---

From: Mark Dilger <[email-masked]>
Date: Fri, 17 May 2024 13:08:10 -0700

> On May 17, 2024, at 12:42 PM, Pavel Borisov <[email-masked]> wrote:
> 

To see if they are detected.

> - how do you know they are not detected?

It appears that they are detected.  At least, rerunning the test after adjusting the expected output, I no longer see problems.

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Fri, 17 May 2024 13:10:46 -0700

> On May 17, 2024, at 1:00 PM, Peter Geoghegan <[email-masked]> wrote:
> 
> especially true for unique indexes. 

The quick-and-dirty TAP test I wrote this morning is intended to introduce duplicates across page boundaries, not to test for ones that got there by normal database activity.  In other words, the TAP test forcibly corrupts the index by changing a value on one side of a boundary to be equal to the value on the other side of the boundary.  Prior to the corrupting action the values were all unique.

—
Mark Dilger

---

From: Peter Geoghegan <[email-masked]>
Date: Fri, 17 May 2024 16:13:20 -0400

> The quick-and-dirty TAP test I wrote this morning is intended to introduce duplicates across page boundaries, not to test for ones that got there by normal database activity.  In other words, the T…

I understood that. I was just pointing out that an index that looks
even somewhat like that is already quite unnatural.

-- 
Peter Geoghegan

---

From: Pavel Borisov <[email-masked]>
Date: Sat, 18 May 2024 01:08:03 +0400

Hi, Mark!

> It appears that they are detected.  At least, rerunning the test after
> adjusting the expected output, I no longer see problems.
>

I understand your point. It was unclear how it modified the index so that
only unique constraint check between pages should have failed with other
checks passed.

Anyway, thanks for your testing and efforts! I'm happy that the test now
passes and confirms that amcheck feature works as intended.

Kind regards,
Pavel Borisov

---

From: Alexander Korotkov <[email-masked]>
Date: Fri, 26 Jul 2024 15:10:32 +0300

> it could be pushed post-FF?  Otherwise, this could wait for v18.

The revised version of 0001 unique checking optimization is attached.
I'm going to push this to v18 if no objections.

------
Regards,
Alexander Korotkov
Supabase

---

From: Robert Haas <[email-masked]>
Date: Fri, 26 Jul 2024 10:38:03 -0400

> I'm going to push this to v18 if no objections.

I have no reason to specifically object to pushing this into 18, but I
would like to point out that you're posting here about this but failed
to reply to the "64-bit pg_notify page numbers truncated to 32-bit",
an open item that was assigned to you but which, since you didn't
respond, was eventually fixed by commits from Michael Paquier.

I know it's easy to lose track of the open items list and I sometimes
forget to check it myself, but it's rather important to stay on top of
any open items that get assigned to you.

Thanks,

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Alexander Korotkov <[email-masked]>
Date: Fri, 26 Jul 2024 23:53:38 +0300

> any open items that get assigned to you.

Yes, it's a pity I miss this open item on me.  Besides putting ashes
on my head, I think I could pay more attention on other open items.

------
Regards,
Alexander Korotkov
Supabase