Repository navigation
pg_rewind: let SUSPEND_PG_REWIND set how long to suspend for - #2176
Open
MisterRaindrop wants to merge 1 commit into
Open
MisterRaindrop wants to merge 1 commit into
MisterRaindrop wants to merge 1 commit into
Conversation
SUSPEND_PG_REWIND is a test-only hook, compiled in under FAULT_INJECTOR, that holds pg_rewind still long enough for a test to catch it in pg_stat_activity and assert that a second gprecoverseg leaves that segment alone. Cloudberry carries an older revision of it: the value has to be the literal string "true", and the sleep is a fixed 120 seconds. Greenplum later made the value the duration. The two scenarios that use this hook set it to "600", which strcmp() rejects, so pg_rewind is never suspended and the step that waits for it to appear gives up: verify that pg_rewind is running for content 0 Exception: Timed out after 600 retries Take the later form, so the caller says how long. Found by running gprecoverseg.feature against a real multi-node cluster, where "gprecoverseg rebalance should be able to terminate gracefully when interrupted by the user" failed on exactly this. With the hook working, that scenario gets much further but is still not reliable on a five-node cluster: it waits only for content 0's pg_rewind before sending SIGTERM, then asserts that the mirrors for content 0 and 1 are both down, and whether content 1 has got that far is a matter of timing. It passed on one run here and failed on two. Waiting for both rewinds first does not fix it -- they are not running at the same time, so the wait itself times out -- and the scenario is identical upstream, so this is left alone rather than papered over. It is @concourse_cluster, which the CI this series adds does not run. cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4, src/bin/pg_rewind/pg_rewind.c. Split out of apache#1639 (originally 735b623).
leborchuk
approved these changes
Oct 23, 2026
leborchuk
left a comment
leborchuk
left a comment
Contributor
There was a problem hiding this comment.
LGTM, fix in FAULT_INJECTOR logic
Alena0704
self-requested a review
October 24, 2026 17:52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
SUSPEND_PG_REWINDis a test-only hook in pg_rewind, compiled in underFAULT_INJECTOR, that holds pg_rewind still so a test can catch it inpg_stat_activityand check that a second gprecoverseg leaves that segment alone. Cloudberry carries the older form, which accepts only the literaltrueand sleeps a fixed 120 seconds. Every caller ingprecoverseg.featuresets it to600, the duration Greenplum later made it, whichstrcmp()rejects, so pg_rewind never suspended and those scenarios time out waiting for it.The value is now the number of seconds. It is parsed with
strtol()and anything that is not a non-negative integer is a fatal error rather than a silent no-op, so a caller still usingtrueis told why instead of hitting a timeout somewhere else. (A negative value would otherwise wrap insleep()'s unsigned argument.)Cherry-picked from Greenplum (greenplum-db/gpdb-archive @ 482967c1b4) with stricter parsing. Split out of #1639 (originally 735b623, by @tuhaihe).
Test Plan
Demo cluster (Rocky 8 build image).
pg_rewind --dry-runagainst a cleanly stopped mirror with its primary as--source-server, which passes the sanity checks and reaches the hook; each run undertimeout 10:SUSPEND_PG_REWINDmain3600(what the scenarios set)true-1,abc, emptyAll callers are in
gprecoverseg.featureand set600; none usestrue, and the Behave step that restores the variable unsets it rather than leaving it empty. Those scenarios are@concourse_cluster(multi-host), which CI does not run, so they were not run here.Type of Change
Checklist