[PATCH v1] amcheck: Allow interrupting the child-level rightlink walk

Started by Paul Kim8 days ago3 messageshackers
Jump to latest
#1Paul Kim
mok03127@gmail.com

bt_child_highkey_check() walks right along the child level following
btpo_next links, reading a page on each iteration, but its loop lacked a
CHECK_FOR_INTERRUPTS(). Every other page-traversal loop in
verify_nbtree.c already has one.

The loop's circular-link detection only catches an immediate two-page
cycle, so a corrupt index whose rightlink chain forms a larger cycle, or
simply a very long forward chain, makes this loop effectively
uninterruptible. As amcheck is specifically meant to be run against
possibly-corrupt indexes, make this walk respond to query cancellation
and shutdown requests the same way the sibling loops do.
---
Hi,

While reading verify_nbtree.c I noticed bt_child_highkey_check()'s
rightlink walk is the only page-traversal loop in the file without a
CHECK_FOR_INTERRUPTS(). Patch attached.

contrib/amcheck/verify_nbtree.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 3ef2d66f826..486bd7a87cc 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -2191,6 +2191,8 @@ bt_child_highkey_check(BtreeCheckState *state,
 	/* Move to the right on the child level */
 	while (true)
 	{
+		CHECK_FOR_INTERRUPTS();
+
 		/*
 		 * Did we traverse the whole tree level and this is check for pages to
 		 * the right of rightmost downlink?
-- 
2.50.1 (Apple Git-155)
#2Paul Kim
mok03127@gmail.com
In reply to: Paul Kim (#1)
[PATCH v2] amcheck: Allow interrupting the child-level rightlink walk

bt_child_highkey_check() walks right along the child level following
btpo_next links, reading a page on each iteration, but its loop lacked a
CHECK_FOR_INTERRUPTS(). Every other page-traversal loop in
verify_nbtree.c already has one.

The loop's existing checks do not guarantee a timely exit on a corrupt
index. The in-loop cycle check only fires when a rightlink points back
through the block the walk started from, or to a page whose btpo_prev
points to itself, so a cycle further downstream can go undetected -- for
example a run of pages all flagged P_INCOMPLETE_SPLIT, which skips both
the high-key comparison and bt_downlink_missing_check() and so never
reaches an error. Such an index, or simply a very long rightlink chain,
makes the walk effectively uninterruptible. Since amcheck exists to be
run against possibly-corrupt indexes, make this walk respond to query
cancellation and shutdown requests like the sibling loops do.
---
v2: only the commit message changed -- v1's description of the in-loop
cycle check was imprecise. The code (the one-line CHECK_FOR_INTERRUPTS)
is unchanged.

This is the same shape as commit 191dce109be ("contrib/amcheck: Add
heapam CHECK_FOR_INTERRUPTS()"), which was backpatched.
bt_child_highkey_check() exists in all supported branches, so this looks
like a backpatch candidate as well.

contrib/amcheck/verify_nbtree.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 3ef2d66f826..486bd7a87cc 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -2191,6 +2191,8 @@ bt_child_highkey_check(BtreeCheckState *state,
 	/* Move to the right on the child level */
 	while (true)
 	{
+		CHECK_FOR_INTERRUPTS();
+
 		/*
 		 * Did we traverse the whole tree level and this is check for pages to
 		 * the right of rightmost downlink?
-- 
2.50.1 (Apple Git-155)
#3Paul Kim
mok03127@gmail.com
In reply to: Paul Kim (#1)
Re: [PATCH v1] amcheck: Allow interrupting the child-level rightlink walk
Show quoted text

Resending the v2 patch as an attachment so cfbot can pick it up — the
earlier copies went inline. No changes to the patch itself.

Attachments:

v2-0001-amcheck-Allow-interrupting-the-child-level-rightl.patchapplication/octet-stream; name=v2-0001-amcheck-Allow-interrupting-the-child-level-rightl.patchDownload+2-1