On Thu, 11 Aug 2022 at 02:03, Euler Taveira <eu...@eulerto.com> wrote: > > On Wed, Aug 10, 2022, at 9:39 AM, osumi.takami...@fujitsu.com wrote: > > Minor review comments for v6. > > Thanks for your review. I'm attaching v7. > > "If the subscriber sets min_apply_delay parameter, ..." > > I suggest we use subscription rather than subscriber, because > this parameter refers to and is used for one subscription. > My suggestion is > "If one subscription sets min_apply_delay parameter, ..." > In case if you agree, there are other places to apply this change. > > I changed the terminology to subscription. I also checked other "subscriber" > occurrences but I don't think it should be changed. Some of them are used as > publisher/subscriber pair. If you think there is another sentence to consider, > point it out. > > It might be better to write a note for committer > like "Bump catalog version" at the bottom of the commit message. > > It is a committer task to bump the catalog number. IMO it is easy to notice > (using a git hook?) that it must bump it when we are modifying the catalog. > AFAICS there is no recommendation to add such a warning. > > The former interprets input number as milliseconds in case of no units, > while the latter takes it as seconds without units. > I feel it would be better to make them aligned. > > In a previous version I decided not to add a code to attach a unit when there > isn't one. Instead, I changed the documentation to reflect what interval_in > uses (seconds as unit). Under reflection, let's use ms as default unit if the > user doesn't specify one. > > I fixed all the other suggestions too.
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. 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. Regards, Vignesh
