Skip to content

interconnect: restart in fast mode so the test does not race crash recovery - #1919

Open
MisterRaindrop wants to merge 2 commits into
apache:mainfrom
MisterRaindrop:fix/interconnect-fast-restart
Open

interconnect: restart in fast mode so the test does not race crash recovery#1919
MisterRaindrop wants to merge 2 commits into
apache:mainfrom
MisterRaindrop:fix/interconnect-fast-restart

Conversation

@MisterRaindrop

Copy link
Copy Markdown
Contributor

What does this PR do?

contrib/interconnect's test restarts the cluster with gpstop -raiq. An
immediate shutdown skips the shutdown checkpoint, so the control file is left in
a state other than DB_SHUTDOWNED and the next startup performs crash recovery:

  • xlogrecovery.c sets InRecovery,
  • xlog.c calls PerformWalRecovery(), which signals PMSIGNAL_RECOVERY_STARTED,
  • the postmaster moves to PM_RECOVERY,
  • and in that state canAcceptConnections() answers CAC_NOTCONSISTENT, reported
    as "the database system is not accepting connections" with detail "Hot standby
    mode is disabled"
    .

gpstart makes exactly such a connection right after pg_ctl returns, to read
the segment configuration, so gpstop -r exits CRITICAL and the restart is
reported as failed.

The damage does not stop at one test. psql gives up at the \c that follows,
so every statement in the file is skipped and the test fails as a whole; the
gpconfig -r shared_preload_libraries cleanup at the end of the file never runs;
and gpstart never got past starting the coordinator in admin mode, so the
cluster is left with no segments up. Suites that run after this one in the
same job then lose their Gather Motion nodes and fail as well — which is what
pg_trgm and indexscan did when this was hit in CI:

-   ->  Gather Motion 3:1  (slice1; segments: 3)
-         Merge Key: ((t <-> 'q0987wertyu0988'::text))
-         ->  Limit
-               ->  Index Scan using trgm_idx on test_trgm
+   ->  Index Scan using trgm_idx on test_trgm

Shut down fast instead. A fast shutdown writes the shutdown checkpoint, the
control file says DB_SHUTDOWNED, no recovery runs, PM_RECOVERY is never
entered, and CAC_NOTCONSISTENT cannot be returned — the failure becomes
unreachable rather than merely less likely.

Fast is also what the rest of the tree already uses: gpstop -raf / -arf
appear in dozens of places, and this file was the only user of -raiq.

Type of Change

  • Bug fix (non-breaking change)
  • Test fix (non-breaking change)
  • New feature (non-breaking change)
  • Breaking change (fix or feature with breaking changes)
  • Documentation update

Test Plan

  • Unit tests added/updated
  • Integration tests added/updated
  • Passed make installcheck
  • Passed make -C src/test installcheck-cbdb-parallel

Measured on a three-segment demo cluster, dirtying 1.5M coordinator rows before
each restart so that recovery is slow enough to lose the race reliably:

result
-raiq (before) failed 2/2, with the message above
-rafq (after) passed 3/3, all three segments still up afterwards

pg_controldata confirms the mechanism at the other end: Database cluster state: in production after an immediate shutdown, shut down after a fast one.
The test itself still passes under pg_regress with the change (ok 1 - interconnect).

Note that a plain reproduction attempt is not reliable — the window is a race,
and on a fast machine gpstart usually wins it. Slowing recovery down (a large
amount of dirty coordinator buffers before the restart) is what makes it
deterministic.

Impact

Performance: the restart is measurably faster, because it no longer pays
for crash recovery: 4s versus 9s in the measurements above.

User-facing changes: none. Test-only.

Dependencies: none.

Checklist

Additional Context

This was originally carried as a commit in #1842, where it was needed to get
that PR's ic-contrib job green. @andr-sokolov asked
why a change to another extension's test was in a datalake_fdw PR, which is a
fair objection — so it is split out here, where it can be judged on its own.

One thing this change does not address: the test has no recovery path if the
restart fails for some other reason, and its cleanup lives inside the
start_ignore block, so a failure still leaves the whole job's cluster without
segments. Removing the -i removes the trigger we actually hit; hardening the
failure path would be a separate change, and I did not want to widen this one.

…covery

The test sets shared_preload_libraries and restarts with "gpstop -raiq".  An
immediate shutdown skips the shutdown checkpoint, so the control file is left
in a state other than DB_SHUTDOWNED and the next startup performs crash
recovery: xlogrecovery.c sets InRecovery, xlog.c calls PerformWalRecovery(),
which signals PMSIGNAL_RECOVERY_STARTED, and the postmaster moves to
PM_RECOVERY.  In that state canAcceptConnections() answers CAC_NOTCONSISTENT,
reported as "the database system is not accepting connections" with detail
"Hot standby mode is disabled".  gpstart makes exactly such a connection right
after pg_ctl returns, to read the segment configuration, so gpstop -r exits
CRITICAL and the restart is reported as failed.

The damage does not stop there.  psql gives up at the \c that follows, so
every statement in the file is skipped and the test fails as a whole; the
cleanup at the end of the file never runs; and gpstart never got past starting
the coordinator in admin mode, so the cluster is left with no segments up.
Suites that run after this one in the same job then lose their Gather Motion
nodes and fail as well.

Shut down fast instead.  A fast shutdown writes the shutdown checkpoint, the
control file says DB_SHUTDOWNED, no recovery runs, PM_RECOVERY is never
entered, and CAC_NOTCONSISTENT cannot be returned -- the failure becomes
unreachable rather than merely less likely.  Fast is also what the rest of the
tree already uses: gpstop -raf/-arf appear in dozens of places, and this file
was the only user of -raiq.

Measured on a three-segment demo cluster, dirtying 1.5M coordinator rows
before each restart so that recovery is slow enough to lose the race
reliably: -raiq failed 2/2 with the message above, -rafq passed 3/3 with all
three segments still up afterwards.  pg_controldata confirms the mechanism at
the other end -- "in production" after an immediate shutdown, "shut down"
after a fast one.  The test still passes under pg_regress with the change.
MisterRaindrop added a commit to MisterRaindrop/cloudberry that referenced this pull request Aug 24, 2026
…crash recovery"

This reverts commit c624a99.

The fix is right but it does not belong here: review asked why a change to
another extension's test was part of a datalake_fdw PR, which is a fair
objection.  It now lives in apache#1919, where it can be judged on its own.

Reverting rather than rewriting the branch, so the review thread that raised
the objection stays anchored to the change it was about.

Consequence worth stating: this PR's ic-contrib job may go red again until
apache#1919 lands, because that race is what made it fail here in the first place.
The failure is in contrib/interconnect and unrelated to this module -- the
library is not in shared_preload_libraries for that job, so it is never even
loaded there.

@leborchuk leborchuk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants