Re: Rework LogicalOutputPluginWriterUpdateProgress

On Mon, 13 Mar 2023 at 08:17, wangw(dot)fnst(at)fujitsu(dot)com
<wangw(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Fri, Mar 10, 2023 20:17 PM Osumi, Takamichi/大墨 昂道 <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> > Hi,
> >
> >
> > On Friday, March 10, 2023 6:32 PM Wang, Wei/王 威 <wangw(dot)fnst(at)fujitsu(dot)com>
> > wrote:
> > > Attach the new patch set.
> > Thanks for updating the patch ! One review comment on v7-0005.
>
> Thanks for your comment.
>
> > stream_start_cb_wrapper and stream_stop_cb_wrapper don't call the pair of
> > threshold check and UpdateProgressAndKeepalive unlike other write wrapper
> > functions like below. But, both of them write some data to the output plugin, set
> > the flag of did_write and thus it updates the subscriber's last_recv_timestamp
> > used for timeout check in LogicalRepApplyLoop. So, it looks adding the pair to
> > both functions can be more accurate, in order to reset the counter in
> > changes_count on the publisher ?
> >
> > @@ -1280,6 +1282,8 @@ stream_start_cb_wrapper(ReorderBuffer *cache,
> > ReorderBufferTXN *txn,
> >
> > /* Pop the error context stack */
> > error_context_stack = errcallback.previous;
> > +
> > + /* No progress has been made, so don't call UpdateProgressAndKeepalive */
> > }
>
> Since I think stream_start/stop_cp are different from change_cb, they don't
> represent records in wal, so I think the LSNs corresponding to these two
> messages are the LSNs of other records. So, we don't call the function
> UpdateProgressAndKeepalive here. Also, for the reasons described in [1].#05, I
> didn't reset the counter here.

As there has been no activity in this thread and it seems there is not
much interest on this from the last 9 months, I have changed the
status of the patch to "Returned with Feedback".

Regards,
Vignesh