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

Hi, Horiguchi-san

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.
Kindly have a look at the patch shared in [1].

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

Best Regards,
Takamichi Osumi