On 2022-Oct-04, Nathan Bossart wrote:
> Here is an updated patch set with the following changes:
>
> * The creation of the struct for non-shared WAL receiver state is moved to
> a prerequisite 0001 patch. This should help ease review of 0002 a bit.
I think that would be even better if you moved the API adjustments of
some functions for the new struct to 0001 as well
(XLogWalRcvSendHSFeedback etc).
> * I updated the nap time calculation to round up to the next millisecond,
> as discussed upthread.
I didn't look at this part very carefully, but IIRC walreceiver's
responsivity has a direct impact on latency for sync replicas. Maybe
it'd be sensible to the definition that the sleep time is rounded down
rather than up? (At least, for the cases where we have something to do
and not merely continue sleeping.)
> * I attempted to minimize the calls to GetCurrentTimestamp(). The WAL
> receiver code already calls this function pretty liberally, so I don't know
> if this is important, but perhaps it could make a difference for systems
> that don't have something like the vDSO to avoid real system calls.
Yeah, we are indeed careful about this in most places. Maybe it's not
particularly important in 2022 and also not particularly important for
walreceiver (again, except in the code paths that affect sync replicas),
but there's no reason not to continue to be careful until we discover
more widely that it doesn't matter.
> * I removed the 'tli' argument from functions that now have an argument for
> the non-shared state struct. The 'tli' is stored within the struct, so the
> extra argument is unnecessary.
+1 (This could also be in 0001, naturally.)
> One thing that still bugs me a little bit about 0002 is that the calls to
> GetCurrentTimestamp() feel a bit scattered and more difficult to reason
> about. AFAICT 0002 keeps 'now' relatively up-to-date, but it seems
> possible that a future change could easily disrupt that. I don't have any
> other ideas at the moment, though.
Hmm.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
