Hi Takeshi, Fujii,
I tested v3 (CF 7407) on master at 94e1385ed28, debug build on Fedora 44, with
the patch applied from the cfbot branch. I'm a sysadmin and have dealt with the
OOM killer and postgres in production, so I wanted to check each claim in the
new text against a real cluster.
1. Child backend killed, restart_after_crash = on
Two sessions running pg_sleep(30), kill -9 on only one of them. The other
session was dropped too:
WARNING: terminating connection because of crash of another server process
Server log:
LOG: client backend (PID 4038002) was terminated by signal 9: Killed
LOG: terminating any other active server processes
LOG: all server processes terminated; reinitializing
...
LOG: database system is ready to accept connections
New connections worked right after. This matches v3, and it also confirms the
current text ("existing database connections will continue to function
normally") is wrong.
2. Same thing with restart_after_crash = off
LOG: terminating any other active server processes
LOG: shutting down because "restart_after_crash" is off
LOG: database system is shut down
Server stays down. Matches v3.
3. Syslogger killed (logging_collector = on)
A running query finished normally, the postmaster started a new syslogger, and
nothing else was terminated. So "typically" is accurate and needed.
4. Postmaster killed
This is the one case where I think the text is missing something. I had two
sessions open, one in pg_sleep(60) and one running a CPU bound query (count(*)
over generate_series(1,400000000)), then kill -9 on the postmaster.
The pg_sleep session was dropped within about a second. The busy one kept
running as an orphan for about 1m42s, finished, returned its result to the
client, and only then exited with:
FATAL: terminating connection due to unexpected postmaster exit
While that orphan was still running, pg_ctl start failed:
FATAL: pre-existing shared memory block (key 555093, ID 131122) is still in use
HINT: Terminate any old server processes associated with data directory "..."
It started fine once the orphan exited.
"All connections will eventually be terminated" is true, but "To recover,
PostgreSQL will need to be restarted" doesn't mention that the restart will
fail until the leftover backends are gone. An admin following the docs during
an incident will hit that FATAL with no explanation. I couldn't find this
mentioned anywhere else in the docs. Something like this after the restart
sentence might help:
Note that the server cannot be restarted until all remaining server
processes have exited; they may need to be terminated manually.
I know Fujii wanted to keep this change narrow, so I'd consider that optional.
5. Docs build
Built the HTML docs with the patch applied, no errors or warnings. The
paragraph renders correctly and the restart_after_crash link resolves.
Minor nit, not worth blocking on: "To recover, PostgreSQL will need to be
restarted." and "If PostgreSQL shuts down, it will need to be restarted to
recover." say nearly the same thing in the same paragraph.
Overall v3 is correct and a clear improvement over the current text. I'm fine
with it going in as is, with or without the extra sentence.
Disclosure: I used an AI assistant (Claude) to help set up the test environment
and draft this email. I ran and checked all the tests myself and I'm happy to
answer questions about any of it.
Thanks for working on this!
-- Clark
