Re: Time based lag tracking for logical replication

On 11/05/17 15:01, Simon Riggs wrote:
> On 11 May 2017 at 08:32, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> On Sun, Apr 23, 2017 at 01:10:32AM +0200, Petr Jelinek wrote:
>>> The time based lag tracking commit [1] added interface for logging
>>> progress of replication so that we can report lag as time interval
>>> instead of just bytes. But the patch didn't contain patch for the
>>> builtin logical replication.
>>>
>>> So I wrote something that implements this.
>>
>> This is listed as a PostgreSQL 10 open item, but the above makes it sound like
>> a feature to consider for v11, not a defect in v10. Why is this an open item?
>
> It's an open item because at the time of the Lag Tracker commit it was
> believed to be a single line of code that needed to be added to
> Logical Replication to make it work with the Lag Tracker
> functionality. It didn't make sense for me to add it while the LR code
> was still being changed, even though we had code to do that.
>
> Petr's new patch is slightly longer and needed review and some minor
> code to add pacing delay.
>
> My own delay in responding has been because of illness. You'll note
> that I'd missed response on at least one other mail from you.
> Apologies for that, it has set me back some way but I'm better now and
> have caught up with other matters. Petr nudged me to look at this
> thread again yesterday, so I had been looking at this over last few
> days.
>
> Attached patch is Petr's patch, slightly rebased with added pacing
> delay, similar to that used by HSFeedback.
>

This looks reasonable. I would perhaps change:
> + /*
> + * Track lag no more than once per WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS
> + */

to something like this for extra clarity:
> + /*
> + * Track lag no more than once per WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS
> + * to avoid flooding the lag tracker on busy servers.
> + */

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services