MIME-Version: 1.0
References: <TYCPR01MB837340F78F4A16F542589195EDFF9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CAJpy0uC2Czf2iJ17yHf4gnMb9pVJyc2QXVe6_NfxiwHhdNLmVg@mail.gmail.com>
 <TYAPR01MB5866D0527B1B8D589F1C2551F5FC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <20230112.120338.534662300633460797.horikyota.ntt@gmail.com>
 <TYCPR01MB83739C6133B50DDA8BAD1601EDFD9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CALDaNm2d=+WBGye015DoACY_PzrRKdmwjeZwDa+Y9LXFqnT0Ow@mail.gmail.com>
 <TYCPR01MB8373F3E780D5BB60E441ECF3EDC69@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CAHut+PsydEoqQaDv861qS_366hBovYAE8RRoDu3=+sBM=yo3Dg@mail.gmail.com>
 <CAHut+PtzoVdm7wj69fKDrh-KLvUHd4scdTunBsQLo97sZNFLGg@mail.gmail.com> <TYCPR01MB8373F5162C7A0E6224670CF0EDC49@TYCPR01MB8373.jpnprd01.prod.outlook.com>
In-Reply-To: <TYCPR01MB8373F5162C7A0E6224670CF0EDC49@TYCPR01MB8373.jpnprd01.prod.outlook.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Fri, 20 Jan 2023 17:55:39 +1100
Message-ID: <CAHut+Puwrb_DEV_7ANSKrQr23-dAmL9vrNV8ww7XsAP+kej0uw@mail.gmail.com>
Subject: Re: Time delayed LR (WAS Re: logical replication restrictions)
To: "Takamichi Osumi (Fujitsu)" <osumi.takamichi@fujitsu.com>
Cc: vignesh C <vignesh21@gmail.com>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, 
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, 
	"shveta.malik@gmail.com" <shveta.malik@gmail.com>, "dilipbalaut@gmail.com" <dilipbalaut@gmail.com>, 
	"amit.kapila16@gmail.com" <amit.kapila16@gmail.com>, "euler@eulerto.com" <euler@eulerto.com>, 
	"m.melihmutlu@gmail.com" <m.melihmutlu@gmail.com>, "andres@anarazel.de" <andres@anarazel.de>, 
	"marcos@f10.com.br" <marcos@f10.com.br>, 
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAHut%2BPuwrb_DEV_7ANSKrQr23-dAmL9vrNV8ww7XsAP%2Bkej0uw%40mail.gmail.com>
Precedence: bulk

Hi Osumi-san, here are my review comments for the latest patch v17-0001.

=3D=3D=3D=3D=3D=3D
Commit Message

1.
Prohibit the combination of this feature and parallel streaming mode.

SUGGESTION (using the same wording as in the code comments)
The combination of parallel streaming mode and min_apply_delay is not allow=
ed.

=3D=3D=3D=3D=3D=3D
doc/src/sgml/ref/create_subscription.sgml

2.
+         <para>
+          By default, the subscriber applies changes as soon as possible. =
This
+          parameter allows the user to delay the application of changes by=
 a
+          specified amount of time. If the value is specified without unit=
s, it
+          is taken as milliseconds. The default is zero (no delay).
+         </para>

Looking at this again, it seemed a  bit strange to repeat "specified"
twice in 2 sentences. Maybe change one of them.

I=E2=80=99ve also suggested using the word "interval" because I don=E2=80=
=99t think
docs yet mentioned anywhere (except in the example) that using
intervals is possible.

SUGGESTION (for the 2nd sentence)
This parameter allows the user to delay the application of changes by
a given time interval.

~~~

3.
+         <para>
+          Any delay occurs only on WAL records for transaction begins afte=
r all
+          initial table synchronization has finished. The delay is calcula=
ted
+          between the WAL timestamp as written on the publisher and the cu=
rrent
+          time on the subscriber. Any overhead of time spent in
logical decoding
+          and in transferring the transaction may reduce the actual wait t=
ime.
+          It is also possible that the overhead already execeeds the reque=
sted
+          <literal>min_apply_delay</literal> value, in which case no addit=
ional
+          wait is necessary. If the system clocks on publisher and subscri=
ber
+          are not synchronized, this may lead to apply changes earlier tha=
n
+          expected, but this is not a major issue because this parameter i=
s
+          typically much larger than the time deviations between servers. =
Note
+          that if this parameter is set to a long delay, the replication w=
ill
+          stop if the replication slot falls behind the current LSN
by more than
+          <link
linkend=3D"guc-max-slot-wal-keep-size"><literal>max_slot_wal_keep_size</lit=
eral></link>.
+         </para>

3a.
Typo "execeeds" (I think Vignesh reported this already)

~

3b.
SUGGESTION (for the 2nd sentence)
BEFORE
The delay is calculated between the WAL timestamp...
AFTER
The delay is calculated as the difference between the WAL timestamp...

~~~

4.
+         <warning>
+           <para>
+            Delaying the replication can mean there is a much longer
time between making
+            a change on the publisher, and that change being
committed on the subscriber.
+            v
+            See <xref linkend=3D"guc-synchronous-commit"/>.
+           </para>
+         </warning>

IMO maybe there is a better way to express the 2nd sentence:

BEFORE
This can have a big impact on synchronous replication.
AFTER
This can impact the performance of synchronous replication.

=3D=3D=3D=3D=3D=3D
src/backend/commands/subscriptioncmds.c

5. parse_subscription_options

@@ -324,6 +328,43 @@ parse_subscription_options(ParseState *pstate,
List *stmt_options,
  opts->specified_opts |=3D SUBOPT_LSN;
  opts->lsn =3D lsn;
  }
+ else if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
+ strcmp(defel->defname, "min_apply_delay") =3D=3D 0)
+ {
+ char    *val,
+    *tmp;
+ Interval   *interval;
+ int64 ms;

IMO 'delay_ms' (or similar) would be a friendlier variable name than just '=
ms'

~~~

6.
@@ -404,6 +445,20 @@ parse_subscription_options(ParseState *pstate,
List *stmt_options,
  "slot_name =3D NONE", "create_slot =3D false")));
  }
  }
+
+ /*
+ * The combination of parallel streaming mode and min_apply_delay is not
+ * allowed.
+ */
+ if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
+ opts->min_apply_delay > 0)
+ {
+ if (opts->streaming =3D=3D LOGICALREP_STREAM_PARALLEL)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s and %s are mutually exclusive options",
+    "min_apply_delay > 0", "streaming =3D parallel"));
+ }

This could be expressed as a single condition using &&, maybe also
with the brackets eliminated. (Unless you feel the current code is
more readable)

~~~

7.

+ if (opts.min_apply_delay > 0)
+ if ((IsSet(opts.specified_opts, SUBOPT_STREAMING) && opts.streaming
=3D=3D LOGICALREP_STREAM_PARALLEL) ||
+ (!IsSet(opts.specified_opts, SUBOPT_STREAMING) && sub->stream =3D=3D
LOGICALREP_STREAM_PARALLEL))
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot enable %s for subscription in %s mode",
+    "min_apply_delay", "streaming =3D parallel"));

These nested ifs could instead be a single "if" with && condition.
(Unless you feel the current code is more readable)


=3D=3D=3D=3D=3D=3D
src/backend/replication/logical/worker.c

8. maybe_delay_apply

+ * Hence, it's not appropriate to apply a delay at the time.
+ */
+static void
+maybe_delay_apply(TimestampTz finish_ts)

That last sentence "Hence,... delay at the time" does not sound
correct. Is there a typo or missing words here?

Maybe it meant to say "... at the STREAM START time."?

~~~

9.
+ /* This might change wal_receiver_status_interval */
+ if (ConfigReloadPending)
+ {
+ ConfigReloadPending =3D false;
+ ProcessConfigFile(PGC_SIGHUP);
+ }

I was unsure why did you make a special mention of
'wal_receiver_status_interval' here. I mean, Aren't there also other
GUCs that might change and affect something here so was there some
special reason only this one was mentioned?

=3D=3D=3D=3D=3D=3D
src/test/subscription/t/032_apply_delay.pl

10.
+
+# Compare inserted time on the publisher with applied time on the subscrib=
er to
+# confirm the latter is applied after expected time.
+sub check_apply_delay_time

Maybe the comment could also mention that the time is automatically
stored in the table column 'c'.

~~~

11.
+# Confirm the suspended record doesn't get applied expectedly by the ALTER
+# DISABLE command.
+$result =3D $node_subscriber->safe_psql('postgres',
+ "SELECT count(a) FROM test_tab WHERE a =3D 0;");
+is($result, qq(0), "check if the delayed transaction doesn't get
applied expectedly");

The use of "doesn't get applied expectedly" (in 2 places here) seemed
strange. Maybe it's better to say like

SUGGESTION
# Confirm disabling the subscription by ALTER DISABLE did not cause
the delayed transaction to be applied.
$result =3D $node_subscriber->safe_psql('postgres',
"SELECT count(a) FROM test_tab WHERE a =3D 0;");
is($result, qq(0), "check the delayed transaction was not applied");

------
Kind Regards,
Peter Smith.
Fujitsu Australia