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

From: Peter Smith <smithpb2250(at)gmail(dot)com>
To: "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>
Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, "andres(at)anarazel(dot)de" <andres(at)anarazel(dot)de>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, "vignesh21(at)gmail(dot)com" <vignesh21(at)gmail(dot)com>, "shveta(dot)malik(at)gmail(dot)com" <shveta(dot)malik(at)gmail(dot)com>, "Takamichi Osumi (Fujitsu)" <osumi(dot)takamichi(at)fujitsu(dot)com>, "dilipbalaut(at)gmail(dot)com" <dilipbalaut(at)gmail(dot)com>, "euler(at)eulerto(dot)com" <euler(at)eulerto(dot)com>, "m(dot)melihmutlu(at)gmail(dot)com" <m(dot)melihmutlu(at)gmail(dot)com>, "marcos(at)f10(dot)com(dot)br" <marcos(at)f10(dot)com(dot)br>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Time delayed LR (WAS Re: logical replication restrictions)
Date: 2023-02-22 01:00:04
Message-ID: CAHut+PvDJYPiYzOdF5PzN=+pwWvAo811VE0nhne9=23SSJrrdA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Here are some very minor review comments for the patch v4-0001

======
Commit Message

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.

~~~

2.
Eariler versions were written by Euler Taveira, Takamichi Osumi, and
Kuroda Hayato

Typo: "Eariler"

======
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"

======
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.

======
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.

======
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.

------
[1] Kuroda-san replied to my review v3-0001.
https://www.postgresql.org/message-id/TYAPR01MB5866C6BCA4D9386D9C486033F5A59%40TYAPR01MB5866.jpnprd01.prod.outlook.com
[2] My previous review v3-0001.
https://www.postgresql.org/message-id/CAHut%2BPu6Y%2BBkYKg6MYGi2zGnx6imeK4QzxBVhpQoPMeDr1npnQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro Horiguchi 2023-02-22 01:03:03 Re: "out of relcache_callback_list slots" after multiple calls to pg_logical_slot_get_binary_changes
Previous Message Nathan Bossart 2023-02-22 00:37:16 Re: Fix the description of GUC "max_locks_per_transaction" and "max_pred_locks_per_transaction" in guc_table.c