Repository navigation
gprecoverseg: port the replay-lag check, --max-rate and stale-slot handling from Greenplum - #2184
Draft
MisterRaindrop wants to merge 7 commits into
Draft
MisterRaindrop wants to merge 7 commits into
MisterRaindrop wants to merge 7 commits into
Conversation
Defects the Behave suite exposes in gprecoverseg:
* gprecoverseg: point a recovered segment's conf at its own port
pg_basebackup and pg_rewind both copy the source segment's
postgresql.conf, port setting included, so a mirror recovered by
gprecoverseg came back configured for its primary's port. On a
multi-host cluster the wrong port is merely wrong; on a single host it
collides with the primary that is still running there.
Rewrite the port before starting the segment, on both the full and the
incremental path, using the existing ModifyConfSetting command -- its
perl expression is anchored at the start of the line, so it touches the
port setting and nothing else. Greenplum does the same in the same two
places. The gpsegrecovery unit tests mock the rewrite, assert it, and
expect the line it logs; before, they ran perl on a postgresql.conf
that does not exist.
Fixes the eight gprecoverseg Behave scenarios that assert
"port value in postgresql.conf of <host> is incorrect".
* gprecoverseg: keep the progress files a recovery writes
Each recovering segment writes pg_basebackup or pg_rewind output to
gpAdminLogs/<tool>.<timestamp>.dbid<N>.out, and gprecoverseg deleted that
file as soon as the segment came back successfully. Deleting it on
success is backwards: those files are what gpstate reads to report
recovery progress, and what an operator reads afterwards to see what a
recovery actually did.
Remove stale progress files for the segments about to be recovered
before starting, and leave the run's own files in place, the way
Greenplum does. Clearing beforehand is also what makes the file a
reliable record of the current run rather than an append of every run
that came before.
The stale files are matched by name ("*dbid<N>.out"), so the remove
command becomes Greenplum's "find <log dir> -name <pattern> -delete".
The old "rm -f" form would have quoted the pattern and run it in the
remote user's home directory, removing nothing. Its unit tests now
assert the single recovery call and the find commands.
Fixes the Behave scenarios asserting gpAdminLogs holds "pg_basebackup*"
and "pg_rewind*" files after a recovery.
* gprecoverseg: reject -F combined with -r or -p
A full resynchronisation rebuilds a mirror from its primary in place.
That is not what -r asks for -- rebalance returns segments to their
preferred roles -- nor what -p asks for, which is to rebuild onto a
different host. gprecoverseg accepted both combinations and silently
did something other than what was asked.
Reject them, with the messages Greenplum uses.
* gprecoverseg: report the failure and the progress files it leaves
Two log lines Greenplum emits around recover_mirrors() were dropped:
- the [ERROR] 'gprecoverseg failed. Please check the output for more
details.' that follows a failed recovery. Without it a failed run
ends on the per-segment errors alone, and callers that scrape the
output -- gpmovemirrors among them -- cannot tell a failure from a
partial success.
- the notice that the pg_basebackup/pg_rewind progress files this run
wrote are left behind and a later gprecoverseg may remove them.
Asserted by gpmovemirrors.feature:136 and gprecoverseg.feature:58.
* gprecoverseg: reject -o combined with -i or -r
-o writes out a sample recovery configuration describing what would be
recovered and recovers nothing. -i names a configuration to recover from
and -r rebalances back to preferred roles, so either one together with -o
is a contradiction: the user asked for a file and for a recovery at the
same time, and gprecoverseg quietly honours one of them.
Upstream rejects both combinations; the checks live in a method whose
other validations are for options this tree does not have, so take just
these two and put them beside the "Only one of -i, -p, and -r" check they
belong with. They run before the cluster is contacted, so the error is
immediate.
gprecoverseg.feature has been asserting this and failing.
* gprecoverseg: say why a segment failed to recover, not just where
When a recovery fails, the summary names the segment and the log file and
stops there:
Failed to recover the following segments. You must run gprecoverseg -F
for all incremental failures
hostname: sdw1; port: 7000; logfile: ~/gpAdminLogs/pg_rewind...out;
recoverytype: incremental
so the one thing the reader wants -- what went wrong -- is the one thing
they have to go and find. Upstream ends that line with "; error: {}" and
fills it by reading the log back, and the printer for setup-recovery
failures in this same file already does the equivalent. Only this one
lost it.
Bring over get_error_from_logfile, which takes the last ERROR, PANIC or
FATAL line from the segment's log. It deliberately skips the line
pg_rewind writes about itself after a failure, which is never the reason
for it; upstream skips PostgreSQL 12's wording, and this tree needs its
own, because pg_rewind.c has since been split into
pg_log_error("postgres single-user mode in target cluster failed");
pg_log_error_detail("Command was: %s", ...);
With the self-referential line out of the way what is left is the
backend's own complaint, which is what a reader needs: for a mirror whose
pg_wal was removed, 'required WAL directory "pg_wal" does not exist'.
The behave step that checks this summary matches up to "recoverytype: X"
with a regex search, so the added field does not disturb it.
Split out of apache#1639 (originally a3bfe47).
gpstate -e decides a recovery is in progress by asking whether
$COORDINATOR_DATA_DIRECTORY/gprecoverseg.lock exists. gprecoverseg
removes that directory when it exits normally, but a process killed with
SIGKILL never gets the chance, so from then on every gpstate -e reports
Segments in recovery
for a recovery that is not happening, and exits 1 while doing so.
Upstream asks a different question: it reads the PID out of the lock
directory and checks whether that process is still alive. Bring over
is_gprecoverseg_running and use it, with the two unit tests that cover
the pid-is-alive and no-pidfile cases.
Found by gprecoverseg.feature:735 "SIGKILL on gprecoverseg should not
display progress in gpstate -e" on a five-node cluster, where it was the
first of 38 failures -- the other 37 were scenarios that inherited the
half-recovered cluster it left behind.
The get_coordinatordatadir import existed only for the directory check
and is now unused, as it is upstream.
Split out of apache#1639 (originally 879b563).
There must be one writer per target segment. If a pg_basebackup or a pg_rewind is already streaming into a segment and gprecoverseg starts a second one, the two races leave the segment inconsistent. Greenplum guards against this by asking gp_stat_replication which segments are already sourcing a pg_basebackup, and pg_stat_activity whether a pg_rewind is connected, then skipping those and saying so. Cloudberry carried over neither check, so a second recovery would just start. Port both. gpmovemirrors needs no change of its own: it moves mirrors by shelling out to gprecoverseg, which is why its three scenarios assert on what gprecoverseg printed. This lets seven scenarios run instead of being skipped -- four in gprecoverseg.feature and three in gpmovemirrors.feature, all of the form "gives warning if pg_basebackup already running for ... segments". cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4, gpMgmt/bin/gppylib/programs/clsRecoverSegment_triples.py. One deliberate departure from upstream: Greenplum skips a third case, a source segment already in backup mode, which only arises from its differential recovery. Cloudberry has no differential recovery, so that arm and its pg_is_in_backup() dependency are left out. Split out of apache#1639 (originally 7056e02).
A segment has both a hostname and an address, and they are not always
the same -- the address is whichever interface the segment listens on,
and a host with several interfaces has several of them. gprecoverseg's
-i config file only understood "address|port|datadir", so on such a
cluster it had to guess the hostname from the address, and a recovery
to a new host could register the segment under the wrong one.
Greenplum lets the line carry the hostname explicitly, as
"hostname|address|port|datadir", and when it does:
- the hostname is cross-checked against the address, by resolving
both and requiring a common IP, so a typo is caught before anything
is recovered rather than after;
- the failed segment is matched on hostname as well, so a line
naming the wrong host no longer silently recovers the right one;
- the error naming an unfound segment quotes all four fields.
Port that. Both groups on a line must use the same number of fields,
which is what the "expected equal parts" error says.
cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4:
gpMgmt/bin/gppylib/programs/clsRecoverSegment_triples.py
gpMgmt/bin/gppylib/utils.py
This lets nine scenarios run instead of being skipped, the
"populates hostname and address from the config file correctly" group
and the two that assert the cross-check warning.
Two deliberate departures from upstream. Greenplum also accepts a fifth
field holding a per-line recovery type (I/D/F), which exists for its
differential recovery; Cloudberry has no differential recovery and no
recovery-type field, so only the 3- and 4-part forms are accepted and
the error message says so. And validateHostnameAddress() calls
socket.gethostbyaddr() rather than importing the bare name, because
gppylib/utils.py already imports the socket module.
One further departure is a fix rather than a trim. Upstream resolves
both names with gethostbyaddr() and requires their address lists to
intersect. gethostbyaddr() answers for a single address family, so on a
dual-stack host "localhost" comes back as ::1 while an address naming
that same machine comes back as 127.0.0.1; the lists do not intersect
and the check reports two different hosts. The consequence is not a
failure but a silence -- the segment is skipped and gprecoverseg still
exits 0, having recovered nothing. Resolve through getaddrinfo() instead
and take every address of each name, so the comparison no longer depends
on which family the resolver happens to answer with first. Observed on
the box this was tested on:
localhost -> 127.0.0.1, ::1
cdw -> 127.0.0.1
which upstream's check calls a mismatch.
Greenplum also reworked how an unreachable failover host is detected
while it was here, probing the hosts in parallel through a new
_get_unreachable_failover_hosts() and threading paralleldegree and
outputConfigFile down for it. That is a separate concern from reading a
hostname out of the config file, and Cloudberry already has its own
unreachable handling that the suite exercises, so this keeps that and
leaves the rework alone.
The -o sample config file still writes three fields, as upstream does.
Split out of apache#1639 (originally 0aa2666).
Rebalancing promotes the mirror and demotes the primary. If the mirror
still has a large amount of WAL left to replay, that promotion stalls
until it catches up, with the segment unavailable for the duration.
Greenplum guards against this by measuring the replay lag before
rebalancing a pair and refusing when it exceeds a limit. Cloudberry
carries the file this lives in but not the check: replay_lag(), the
block in rebalance() that calls it, the ALLOWED_REPLAY_LAG default and
the two options that drive it had all been dropped.
Port them back, so these two scenarios can run instead of being skipped:
gprecoverseg rebalance aborts and throws exception if replay lag on
mirror is more than or equal to the allowed limit
gprecoverseg errors out if invalid options are used with
--disable-replay-lag
cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4:
gpMgmt/bin/gppylib/operations/rebalanceSegments.py
gpMgmt/bin/gppylib/programs/clsRecoverSegment.py
gpMgmt/bin/gppylib/commands/gp.py
gpMgmt/bin/gppylib/test/unit/test_unit_rebalance_segment.py
The one deliberate departure from upstream: Greenplum reaches the same
check through a differential-recovery code path that Cloudberry does not
have, so only the rebalance caller is ported.
Split out of apache#1639 (originally ad3c9d5).
A full recovery runs pg_basebackup, which will use whatever network
bandwidth it can get. On a cluster that is still serving queries that
can starve everything else, so Greenplum lets the operator cap it with
--max-rate, validated and then passed through to pg_basebackup.
Cloudberry has the same plumbing -- gprecoverseg builds a
GpMirrorListToBuild, which builds a GpSegRecovery command line, which
gpsegrecovery parses and turns into a PgBaseBackup -- but no maxRate
parameter anywhere along it. Add it at each hop, along with the
validation, the two "this flag is ignored with -r/-o" warnings, and the
"Maximum Transfer Rate" row in the recovery plan the scenario asserts on.
This lets the scenario run instead of being skipped:
gprecoverseg recovers segment for valid max-rate options and errors
out for others
cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4:
gpMgmt/bin/gppylib/programs/clsRecoverSegment.py
gpMgmt/bin/gppylib/operations/buildMirrorSegments.py
gpMgmt/bin/gppylib/commands/gp.py
gpMgmt/bin/gppylib/commands/pg.py
gpMgmt/sbin/gpsegrecovery.py
gpMgmt/sbin/recovery_base.py
Two deliberate departures from upstream. Greenplum also threads maxRate
into its differential recovery path, which Cloudberry does not have.
And Cloudberry's FullRecovery retries pg_basebackup with --create-slot
when the first attempt fails on a cluster that never had mirrors;
upstream has no such retry, so the cap is applied to it too -- without
that the retry would silently ignore the limit the operator asked for.
The comment in gprecoverseg.feature that claimed Greenplum does not
implement this either was wrong; it goes when apache#2170 drops the tag.
Split out of apache#1639 (originally 993b99d).
… retry
A full recovery streams from the primary through the replication slot
internal_wal_replication_slot. pg_basebackup --create-slot fails if that
slot already exists, and PostgreSQL offers no way to reuse one.
Cloudberry worked around it by guessing: run pg_basebackup without
--create-slot, and if anything at all goes wrong, run the whole thing
again with it. That is expensive when the backup failed for an unrelated
reason -- the second attempt also forces forceoverwrite=True, discarding
the target directory -- and it leaves the slot handling implicit. The
code says as much:
GPDB_12_MERGE_FIXME could we check it before? or let
pg_basebackup create slot if not exists.
Greenplum answers that FIXME: ask the primary whether the slot is there,
drop it if so, and pass --create-slot once. Port PgReplicationSlot and
use it, in gpsegrecovery for gprecoverseg and in gpconfigurenewsegment
for gpexpand, dropping both retries.
Porting it into gpconfigurenewsegment is not optional. PgBaseBackup's
constructor now talks to the database, and there the second constructor
call sat inside an except block, so a failure to reach the primary would
have escaped from the handler rather than being reported.
This lets the last two skipped scenarios run, which also assert the
messages this prints:
gprecoverseg should drop existing slot on full recovery
<scenario> recovery should not try to drop slot if slot does not exist
With them, nothing in gprecoverseg.feature or gpmovemirrors.feature is
tagged @not_implemented any more.
cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4:
gpMgmt/bin/gppylib/commands/pg.py
gpMgmt/sbin/gpsegrecovery.py
gpMgmt/bin/lib/gpconfigurenewsegment
Two things in gpconfigurenewsegment are fixed in passing, both found by
running open-code-review over this commit:
- It called PgBaseBackup with logfile=, a keyword that constructor has
never had, so the mirror path raised TypeError before doing anything.
Upstream has the same line and the same bug; nothing in either test
suite reaches it, because gpaddmirrors builds mirrors through
gpsegrecovery and only gpexpand comes this way. It is progress_file=.
- The constructor call sat outside the try below. Now that it talks to
the database it can fail, and a failure there returned no result at
all, which the worker pool reports as "'NoneType' object has no
attribute 'wasSuccessful'" long after the fact.
One deliberate departure from upstream: drop_slot() catches pgdb's
DatabaseError, not psycopg2's, because that is the driver Cloudberry's
dbconn is built on.
Split out of apache#1639 (originally 8377856).
This was referenced Oct 24, 2026
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?
Ports three gprecoverseg features from Greenplum that the gpMgmt Behave suite (#2170) has scenarios for. Cloudberry carried the scenarios but not the code, so they had to be skipped. Three commits; each commit message gives the full reasoning and lists what is cherry-picked from greenplum-db/gpdb-archive @ 482967c1b4.
gprecoverseg -rpromotes a mirror, it measures how much WAL the mirror still has to replay, and refuses when that exceeds a limit, because the promotion would stall until replay caught up. This brings backreplay_lag(), the block inrebalance()that calls it, theALLOWED_REPLAY_LAGdefault, and the--replay-lagand--disable-replay-lagoptions.--max-rate. This caps pg_basebackup's bandwidth during a full recovery so it does not starve a cluster that is still serving queries. The value is validated and passed along gprecoverseg →GpMirrorListToBuild→gpsegrecovery→PgBaseBackup.pg_basebackup --create-slotfails ifinternal_wal_replication_slotalready exists. Cloudberry worked around that by running pg_basebackup without the flag and, on any failure, running the whole backup again with it and withforceoverwrite=True. The code's ownGPDB_12_MERGE_FIXMEasked for something better. Greenplum's answer is ported: ask the primary whether the slot exists, drop it if it does, and pass--create-slotonce. This applies ingpsegrecovery(gprecoverseg) and ingpconfigurenewsegment(gpexpand). Two bugs ingpconfigurenewsegmentare fixed in passing; the commit message has the details.Where each commit departs from upstream, its message says so and why: Greenplum's differential-recovery path does not exist here, and
drop_slot()catches PyGreSQL'sDatabaseErrorrather than psycopg2's.This builds on #2183 (gprecoverseg fixes): commit 3 touches the code that PR changes, and the scenarios assume its fixes. Until #2183 merges, this PR also shows that PR's four commits, so please review only the last three here. I am keeping this PR as a draft until then. Once it merges, I will rebase this PR and mark it ready.
Split out of #1639 (originally ad3c9d5, 993b99d and 8377856, by @tuhaihe).
Changes from #1639:
test_complete_workflow_exceptionstill expected pg_basebackup to fail twice, which commit 3 makes impossible. It failed once CI: add gpMgmt Behave test #1639's other test changes were in. It now expects one attempt and one error.@not_implementedtoday. That stops being true once CI: run the gpMgmt Behave suite #2170 tags the gpcheckcat-xscenario, so that edit is left out.Why one PR for three ports: #1639 carried 19 fixes. Opening one PR per fix would leave a long queue of tiny PRs for reviewers, so the remaining ones are grouped by area into five PRs (this one, #2178, #2180, #2183 and #2182). The commits are kept separate on purpose, so please merge this with "Rebase and merge" rather than squash.
Test Plan
Rocky 9 build image, built from #2170's tree plus #2177, #2178, #2180 and #2183, with a demo cluster created the way the Behave workflow creates it. In
gprecoverseg.featureI untagged the scenarios these three commits make runnable, then ran it with CI's arguments.gprecoverseg.feature(--tags ~@concourse_cluster --tags ~@extended --tags ~@differential --tags ~@not_implemented)gpmovemirrors.feature(--tags ~@concourse_cluster --tags ~@extended)gpaddmirrors.feature(--tags ~@concourse_cluster)gpmovemirrors and gpaddmirrors are there to show nothing regressed. Both build mirrors through the full-recovery path in
gpsegrecoverythat commit 3 changes.Without this PR, every one of these scenarios that runs fails:
Checking if slot internal_wal_replication_slot exists.error: transfer rate 30 is out of range.The max-rate failure leaves the mirrors down, so the next scenario fails on
all segments are not currently running, and the one after waits forever onuser can start transactions. That is why run A timed out. The two replay-lag scenarios come later in the file and were never reached. #2170 therefore tags these five scenarios@needs_fixone by one, so a missing feature cannot hang the rest of the job.Unit tests (
python3 -m unittest, withPYTHONPATHset the waymake -C gpMgmt/bin unitdevelsets it):test_unit_rebalance_segment,test_unit_gpsegrecoveryandtest_unit_pg_base_backuppass. Across the wholegppylibsuite, this PR adds no failures. It fixes threetest_unit_rebalance_segmenttests that fail without it, because they test the replay-lag check. Intest_unit_gprecoverseg,test_gprecoverseg_with_mirrorlesserrors with and without this PR: its mockgpArrayhas nostandbyCoordinator. I have not looked into the other tests that fail either way.#2170 tags the scenarios these commits make runnable
@needs_fix, each pointing at its #1639 commit. Whichever PR lands second removes those tags.Type of Change
Checklist