Re: Rework LogicalOutputPluginWriterUpdateProgress

On Sat, Feb 11, 2023 at 2:34 AM Andres Freund <andres@anarazel.de> wrote:
>
> On 2023-02-09 11:21:41 +0530, Amit Kapila wrote:
> > On Thu, Feb 9, 2023 at 1:33 AM Andres Freund <andres@anarazel.de> wrote:
> > >
> > > Hacking on a rough prototype how I think this should rather look, I had a few
> > > questions / remarks:
> > >
> > > - We probably need to call UpdateProgress from a bunch of places in decode.c
> > >   as well? Indicating that we're lagging by a lot, just because all
> > >   transactions were in another database seems decidedly suboptimal.
> > >
> >
> > We can do that but I think in all those cases we will reach quickly
> > enough back to walsender logic (WalSndLoop - that will send keepalive
> > if required) that we don't need to worry. After processing each
> > record, the logic will return back to the main loop that will send
> > keepalive if required.
>
> For keepalive processing yes, for syncrep and accurate lag tracking, I don't
> think that suffices?  We could do that in WalSndLoop() instead I guess, but
> we'd have more information about when that's useful in decode.c.
>

Yeah, I think one possibility to address that is to call
update_progress() in DecodeCommit() and friends when we need to skip
the xact. We decide that in DecodeTXNNeedSkip. In the checks in that
function, I am not sure whether we need to call it for the case where
we skip the xact because we decide that it was previously decoded.

>
> > The patch calls update_progress in change_cb_wrapper and other
> > wrappers which will miss the case of DDLs that generates a lot of data
> > that is not processed by the plugin. I think for that we either need
> > to call update_progress from reorderbuffer.c similar to what the patch
> > has removed or we need some other way to address it. Do you have any
> > better idea?
>
> I don't mind calling something like update_progress() in the specific cases
> that's needed, but I think those are just the
>   if (!RelationIsLogicallyLogged(relation))
>   if (relation->rd_rel->relrewrite && !rb->output_rewrites))
>
> To me it makes a lot more sense to call update_progress() for those, rather
> than generally.
>

Won't it be better to call it wherever we don't invoke any wrapper
function like for cases REORDER_BUFFER_CHANGE_INVALIDATION, sequence
changes, etc.? I was thinking that wherever we don't call the wrapper
function which means we don't have a chance to invoke
update_progress(), the timeout can happen if there are a lot of such
messages.

>
> I think, independent of the update_progress calls, it'd be worth investing a
> bit of time into optimizing those cases, so that we don't put the changes into
> the reorderbuffer in the first place.  I think we find space for two flag bits
> to identify the cases in the WAL, rather than needing to access the catalog to
> figure it out.  If we don't find space, we could add an annotation the WAL
> record (making it bigger) for the two cases, because they're not the path most
> important to optimize.
>
>
>
> > > - Why should lag tracking only be updated at commit like points? That seems
> > >   like it adds odd discontinuinities?
> > >
> >
> > We have previously experimented to call it from non-commit locations
> > but that turned out to give inaccurate information about Lag. See
> > email [1].
>
> That seems like an issue with WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS, not with
> reporting something more frequently.  ISTM that
> WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS just isn't a good proxy for when to
> update lag reporting for records that don't strictly need it. I think that
> decision should be made based on the LSN, and be deterministic.
>
>
> > > - Aren't the wal_sender_timeout / 2 checks in WalSndUpdateProgress(),
> > >   WalSndWriteData() missing wal_sender_timeout <= 0 checks?
> > >
> >
> > It seems we are checking that via
> > ProcessPendingWrites()->WalSndKeepaliveIfNecessary(). Do you think we
> > need to check it before as well?
>
> Either we don't need the precheck at all, or we should do it reliably. Right
> now we'll have a higher overhead / some behavioural changes, if
> wal_sender_timeout is disabled. That doesn't make sense.
>

Fair enough, we can probably do it earlier.

>
> > How about renaming ProcessPendingWrites to WaitToSendPendingWrites or
> > WalSndWaitToSendPendingWrites?
>
> I don't like those much:
>
> We're not really waiting for the data to be sent or such, we just want to give
> it to the kernel to be sent out. Contrast that to WalSndWaitForWal, where we
> actually are waiting for something to complete.
>
> I don't think 'write' is a great description either, although our existing
> terminology is somewhat muddled. We're waiting calling pq_flush() until
> !pq_is_send_pending().
>
> WalSndSendPending() or WalSndFlushPending()?
>

Either of those sounds fine.

-- 
With Regards,
Amit Kapila.