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

Dear Dilip,

Thanks for reviewing our patch! PSA new version patch set.
Again, 0001 is not made by us, brought from [1].

> I have done some review for the patch and I have a few comments.
>
> 1.
> A.
> + <literal>wal_sender_timeout</literal> on the publisher. Otherwise, the
> + walsender repeatedly terminates due to timeout during the delay of
> + the subscriber.
>
>
> B.
> +/*
> + * In order to avoid walsender's timeout during time delayed replication,
> + * it's necessaary to keep sending feedbacks during the delay from the worker
> + * process. Meanwhile, the feature delays the apply before starting the
> + * transaction and thus we don't write WALs for the suspended changes during
> + * the wait. Hence, in the case the worker process sends a feedback during the
> + * delay, avoid having positions of the flushed and apply LSN overwritten by
> + * the latest LSN.
> + */
>
> - Seems like these two statements are conflicting, I mean if we are
> sending feedback then why the walsender will timeout?

It is a possibility that timeout is occurred because the interval between feedback
messages may become longer than wal_sender_timeout. Reworded and added descriptions.

> - Typo /necessaary/necessary

Fixed.

> 2.
> + *
> + * During the time delayed replication, avoid reporting the suspeended
> + * latest LSN are already flushed and written, to the publisher.
> */
> Typo /suspeended/suspended

Fixed.

> 3.
> + if (wal_receiver_status_interval > 0
> + && diffms > wal_receiver_status_interval)
> + {
> + WaitLatch(MyLatch,
> + WL_LATCH_SET | WL_TIMEOUT |
> WL_EXIT_ON_PM_DEATH,
> + (long) wal_receiver_status_interval,
> + WAIT_EVENT_RECOVERY_APPLY_DELAY);
> + send_feedback(last_received, true, false);
> + }
> + else
> + WaitLatch(MyLatch,
> + WL_LATCH_SET | WL_TIMEOUT |
> WL_EXIT_ON_PM_DEATH,
> + diffms,
> + WAIT_EVENT_RECOVERY_APPLY_DELAY);
>
> I think here we should add some comments to explain about sending
> feedback, something like what we have explained at the time of
> defining the "in_delaying_apply" variable.

Added.

> 4.
>
> + * Although the delay is applied in BEGIN messages, streamed transactions
> + * apply the delay in a STREAM COMMIT message. That's ok because no
> + * changes have been applied yet (apply_spooled_messages() will do it).
> + * The STREAM START message would be a natural choice for this delay
> but
> + * there is no commit time yet (it will be available when the in-progress
> + * transaction finishes), hence, it was not possible to apply a delay at
> + * that time.
> + */
> + maybe_delay_apply(commit_data.committime);
>
> I am wondering how this will interact with the parallel apply worker
> where we do not spool the data in file? How are we going to get the
> commit time of the transaction without applying the changes?

We think that parallel apply workers should not delay applications because if
they delay transactions before committing they may hold locks very long time.

> 5.
> + /*
> + * The following operations use these special functions to detect
> + * overflow. Number of ms per informed days.
> + */
>
> This comment doesn't make much sense, I think this needs to be rephrased.

Changed to simpler expression.

We have also fixed wrong usage of wal_receiver_status_interval. We must convert
the unit from [s] to [ms] when it is passed to WaitLatch().

Note that more than half of the modifications are done by Osumi-san.

[1]: https://www.postgresql.org/message-id/20221215224721.GA694065%40nathanxps13

Best Regards,
Hayato Kuroda
FUJITSU LIMITED