Dear Peter, Thank you for reviewing! PSA new version.
> 1.
> The other possibility was to apply the delay at the end of the parallel apply
> transaction but that would cause issues related to resource bloat and
> locks being
> held for a long time.
>
> ~
>
> The reply [1] for review comment #2 says that this was "slightly
> reworded", but AFAICT nothing is changed here.
Oh, my git operation might be wrong and it was disappeared.
Sorry for inconvenience, reworded again.
> 2.
> Eariler versions were written by Euler Taveira, Takamichi Osumi, and
> Kuroda Hayato
>
> Typo: "Eariler"
Fixed.
> ======
> doc/src/sgml/ref/create_subscription.sgml
>
> 3.
> + <para>
> + By default, the publisher sends changes as soon as possible. This
> + parameter allows the user to delay changes by given time period. If
> + the value is specified without units, it is taken as milliseconds.
> + The default is zero (no delay). See <xref
> linkend="config-setting-names-values"/>
> + for details on the available valid time units.
> + </para>
>
> "by given time period" --> "by the given time period"
Fixed.
> src/backend/replication/pgoutput/pgoutput.c
>
> 4. parse_output_parameters
>
> + else if (strcmp(defel->defname, "min_send_delay") == 0)
> + {
> + unsigned long parsed;
> + char *endptr;
>
> I think 'parsed' is a fairly meaningless variable name. How about
> calling this variable something useful like 'delay_val' or
> 'min_send_delay_value', or something like those? Yes, I recognize that
> you copied this from some existing code fragment, but IMO that doesn't
> make it good.
OK, changed to 'delay_val'.
>
> ======
> src/backend/replication/walsender.c
>
> 5.
> + /* Sleep until we get reply from worker or we time out */
> + WalSndWait(WL_SOCKET_READABLE,
> + Min(timeout_sleeptime_ms, remaining_wait_time_ms),
> + WAIT_EVENT_WALSENDER_SEND_DELAY);
>
> In my previous review [2] comment #14, I questioned if this comment
> was correct. It looks like that was accidentally missed.
Sorry, I missed that. But I think this does not have to be changed.
Important point here is that WalSndWait() is used, not WaitLatch().
According to comment atop WalSndWait(), the function waits till following
events:
- the socket becomes readable or writable
- a timeout occurs
Logical walsender process is always connected to worker, so the socket becomes
readable
when apply worker sends feedback message.
That's why I wrote "Sleep until we get reply from worker or we time out".
> src/include/replication/logical.h
>
> 6.
> + /*
> + * The minimum delay, in milliseconds, by the publisher before sending
> + * COMMIT/PREPARE record
> + */
> + int32 min_send_delay;
>
> The comment is missing a period.
Right, added.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v5-0001-Time-delayed-logical-replication-on-publisher-sid.patch
Description: v5-0001-Time-delayed-logical-replication-on-publisher-sid.patch
