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

On Wednesday, January 25, 2023 3:55 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> 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.
Oh, sorry for that. I have made the change back.
Kindly have a look at the v22 shared in [1].

[1] - https://www.postgresql.org/message-id/TYCPR01MB837305BD31FA317256BC7B1FEDCE9%40TYCPR01MB8373.jpnprd01.prod.outlook.com

Best Regards,
Takamichi Osumi