From 80ec2b16db6d15f039ec715a52698cc5a22a53f3 Mon Sep 17 00:00:00 2001 From: Dianjin Wang Date: Sat, 10 Oct 2026 11:51:31 +0800 Subject: [PATCH] pg_rewind: let SUSPEND_PG_REWIND set how long to suspend for 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 #1639 (originally 735b623d56e). --- src/bin/pg_rewind/pg_rewind.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index e0ee7930046..6ceae687e69 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -10,6 +10,7 @@ #include "postgres_fe.h" #include #include +#include #include #include @@ -368,16 +369,34 @@ main(int argc, char **argv) #ifdef FAULT_INJECTOR /* - * SUSPEND_PG_REWIND is used for testing purposes. If set to true, the pg_rewind process will be - * suspended for 120 seconds, so that it's entry can be checked in the pg_stat_activity table. + * SUSPEND_PG_REWIND is used for testing purposes. If set to an int, the pg_rewind process will be + * suspended for set amount of time(secs), so that it's entry can be checked in the pg_stat_activity table. * The goal being that we can run another instance of gprecoverseg and assert that it ignores * recovery for segments that already have an active pg_rewind process. */ char* suspend_pg_rewind = getenv("SUSPEND_PG_REWIND"); - if(suspend_pg_rewind != NULL && strcmp(suspend_pg_rewind, "true") == 0) + if(suspend_pg_rewind != NULL) { - pg_log_info("pg_rewind suspended"); - sleep(120); + char *end; + long secs; + + /* + * Not atoi(): it reports 0 for anything it cannot parse. This + * variable used to be the literal string "true", so a caller that + * has not caught up would get a silent no-op and then a timeout in + * whatever was waiting to observe the suspended process -- with + * nothing anywhere saying why. A negative value is worse: sleep() + * takes an unsigned int, so -1 becomes a sleep of some decades. + */ + errno = 0; + secs = strtol(suspend_pg_rewind, &end, 10); + if (*suspend_pg_rewind == '\0' || *end != '\0' || + errno != 0 || secs < 0 || secs > INT_MAX) + pg_fatal("SUSPEND_PG_REWIND must be a non-negative number of seconds, not \"%s\"", + suspend_pg_rewind); + + pg_log_info("pg_rewind suspended for %ld seconds", secs); + sleep((unsigned int) secs); } #endif