Re: Time delayed LR (WAS Re: logical replication restrictions)

On Wed, Jan 25, 2023 at 11:23 AM Takamichi Osumi (Fujitsu)
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
>
> Thank you for checking the patch !
> On Wednesday, January 25, 2023 10:17 AM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> > In short, I'd like to propose renaming the parameter in_delayed_apply of
> > send_feedback to "has_unprocessed_change".
> >
> > At Tue, 24 Jan 2023 12:27:58 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> > wrote in
> > > > send_feedback():
> > > > + * If the subscriber side apply is delayed (because of
> > time-delayed
> > > > + * replication) then do not tell the publisher that the received
> > latest
> > > > + * LSN is already applied and flushed, otherwise, it leads to the
> > > > + * publisher side making a wrong assumption of logical
> > replication
> > > > + * progress. Instead, we just send a feedback message to avoid a
> > publisher
> > > > + * timeout during the delay.
> > > > */
> > > > - if (!have_pending_txes)
> > > > + if (!have_pending_txes && !in_delayed_apply)
> > > > flushpos = writepos = recvpos;
> > > >
> > > > Honestly I don't like this wart. The reason for this is the function
> > > > assumes recvpos = applypos but we actually call it while holding
> > > > unapplied changes, that is, applypos < recvpos.
> > > >
> > > > Couldn't we maintain an additional static variable "last_applied"
> > > > along with last_received?
> > > >
> > >
> > > It won't be easy to maintain the meaning of last_applied because there
> > > are cases where we don't apply the change directly. For example, in
> > > case of streaming xacts, we will just keep writing it to the file,
> > > now, say, due to some reason, we have to send the feedback, then it
> > > will not allow you to update the latest write locations. This would
> > > then become different then what we are doing without the patch.
> > > Another point to think about is that we also need to keep the variable
> > > updated for keep-alive ('k') messages even though we don't apply
> > > anything in that case. Still, other cases to consider are where we
> > > have mix of streaming and non-streaming transactions.
> >
> > Yeah. Even though I named it as "last_applied", its objective is to have
> > get_flush_position returning the correct have_pending_txes without a hint
> > from callers, that is, "let g_f_position know if store_flush_position has been
> > called with the last received data".
> >
> > Anyway I tried that but didn't find a clean and simple way. However, while on it,
> > I realized what the code made me confused.
> >
> > +static void send_feedback(XLogRecPtr recvpos, bool force, bool
> > requestReply,
> > + bool in_delayed_apply);
> >
> > The name "in_delayed_apply" doesn't donsn't give me an idea of what the
> > function should do for it. If it is named "has_unprocessed_change", I think it
> > makes sense that send_feedback should think there may be an outstanding
> > transaction that is not known to the function.
> >
> >
> > So, my conclusion here is I'd like to propose changing the parameter name to
> > "has_unapplied_change".
> Renamed the variable name to "has_unprocessed_change".
> Also, removed the first argument of the send_feedback() which isn't necessary now.
>

Why did you remove the first argument of the send_feedback() when that
is not added by this patch? If you really think that is an
improvement, feel free to propose that as a separate patch.
Personally, I don't see a value in it.

--
With Regards,
Amit Kapila.