Well, since I am listed as a reviewer, I might as well do a real review. :)
> It is disabled by default. This parameter can be set for each walsender.
Not sure what this means, it seems to imply you can tweak already-created
walsenders. Remove or reword.
> WalSndCheckTimeOut
Should be "Timeout" everywhere (timeout is one word, not two)
> if (!(got_STOPPING || got_SIGUSR2))
I'm not really clear on this - wouldn't we want this only for got_SIGUSR2?
> /* Try to inform receiver that XLOG streaming is done */
> SetQueryCompletion(&qc, CMDTAG_COPY, 0);
> EndCommand(&qc, DestRemote, false);
Is it possible to get here without being in a copy state? I'm worried about
unconditionally sending this.
> (errmsg("walsender shutting down due to wal_sender_shutdown_timeout
expiration"),
Any way to give more context here? Like tell if things were buffered, or
where we were in the stream?
Also more standardly written as "terminating walsender process due to
wal_sender_shutdown_timeout"
> errhint("Replication may be incomplete.")));
Is that a hint? Seems it should be part of the warning.
> + long_desc => '-1 disables timeout',
Worth a "0 means ..." like some other GUCs do?
> # 0 sets immediate termination of walsender
Better as "0 means immediate termination of walsender"
> WalSndDoneImmediate()
Add (void) to match the declaration
Tests:
* Should test something other than -1 and 0 (e.g. a positive value)
* No checking that the WARNING/HINT is emitted
* s/with wal_sender_shutdown_timeout is set/with
wal_sender_shutdown_timeout set/
* Import PostgreSQL::Test::Utils at the top
* pass('successfull fast shutdown of server with empty output buffers'); <<
are they really empty?
Three places have 'successfull' which should be 'successful'
Two places have 'receival' which should be 'receipt'
> * This waiting time can be limited by wal_sender_shutdown_timeout
parameter.
s/limited by /limited by the /
Cheers,
Greg
