MisterRaindrop opened a new pull request, #1919:
URL: https://github.com/apache/cloudberry/pull/1919
<!-- Thank you for your contribution to Apache Cloudberry (Incubating)! -->
### 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)
- [x] 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
- [x] Integration tests added/updated
- [x] 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
- [ ] Followed [contribution
guide](https://cloudberry.apache.org/contribute/code)
- [ ] Added/updated documentation
- [x] Reviewed code for security implications
- [x] This PR contains AI-assisted code generation
- [ ] Requested review from [cloudberry
committers](https://github.com/orgs/apache/teams/cloudberry-committers)
### 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](https://github.com/apache/cloudberry/pull/1842#discussion_r3828714300)
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
