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

On Mon, Nov 14, 2022 at 12:14 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sat, Nov 12, 2022 at 7:21 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Few comments:
> > 1) I feel if the user has specified a long delay there is a chance
> > that replication may not continue if the replication slot falls behind
> > the current LSN by more than max_slot_wal_keep_size. I feel we should
> > add this reference in the documentation of min_apply_delay as the
> > replication will not continue in this case.
> >
>
> This makes sense to me.
>
> > 2) I also noticed that if we have to shut down the publisher server
> > with a long min_apply_delay configuration, the publisher server cannot
> > be stopped as the walsender waits for the data to be replicated. Is
> > this behavior ok for the server to wait in this case? If this behavior
> > is ok, we could add a log message as it is not very evident from the
> > log files why the server could not be shut down.
> >
>
> I think for this case, the behavior should be the same as for physical
> replication. Can you please check what is behavior for the case you
> are worried about in physical replication? Note, we already have a
> similar parameter for recovery_min_apply_delay for physical
> replication.
>

I don't understand the reason for the below change in the patch:

+ /*
+ * If this subscription has been disabled and it has an apply
+ * delay set, wake up the logical replication worker to finish
+ * it as soon as possible.
+ */
+ if (!opts.enabled && sub->applydelay > 0)
+ logicalrep_worker_wakeup(sub->oid, InvalidOid);
+

It seems to me Kuroda-San has proposed this change [1] to fix the test
but it is not clear to me why such a change is required. Why can't
CHECK_FOR_INTERRUPTS() after waiting, followed by the existing below
code [2] in LogicalRepApplyLoop() sufficient to handle parameter
updates?

[2]
if (!in_remote_transaction && !in_streamed_transaction)
{
/*
* If we didn't get any transactions for a while there might be
* unconsumed invalidation messages in the queue, consume them
* now.
*/
AcceptInvalidationMessages();
maybe_reread_subscription();
...

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

--
With Regards,
Amit Kapila.