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>
 <CAHut+Puwrb_DEV_7ANSKrQr23-dAmL9vrNV8ww7XsAP+kej0uw@mail.gmail.com>
 <TYCPR01MB8373BED9E390C4839AF56685EDC59@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <TYCPR01MB8373A23EFC25B22CB132383AEDCB9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
In-Reply-To: <TYCPR01MB8373A23EFC25B22CB132383AEDCB9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Mon, 23 Jan 2023 17:36:13 +0530
Message-ID: <CAA4eK1JK+3hO2Ki4h2bkZazyKHtTzvD77V5DZqFdTUNDtdocvQ@mail.gmail.com>
Subject: Re: Time delayed LR (WAS Re: logical replication restrictions)
To: "Takamichi Osumi (Fujitsu)" <osumi.takamichi@fujitsu.com>
Cc: Peter Smith <smithpb2250@gmail.com>, 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>, 
	"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"
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1JK%2B3hO2Ki4h2bkZazyKHtTzvD77V5DZqFdTUNDtdocvQ%40mail.gmail.com>
Precedence: bulk

On Sun, Jan 22, 2023 at 6:12 PM Takamichi Osumi (Fujitsu)
<osumi.takamichi@fujitsu.com> wrote:
>
>
> Attached the updated patch v19.
>

Few comments:
=============
1.
}
+
+
+/*

Only one empty line is sufficient between different functions.

2.
+ if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
+ opts->min_apply_delay > 0 && opts->streaming == LOGICALREP_STREAM_PARALLEL)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s and %s are mutually exclusive options",
+    "min_apply_delay > 0", "streaming = parallel"));
 }

I think here we should add a comment for the translator as we are
doing in some other nearby cases.

3.
+ /*
+ * The combination of parallel streaming mode and
+ * min_apply_delay is not allowed.
+ */
+ if (opts.streaming == LOGICALREP_STREAM_PARALLEL)
+ if ((IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) &&
opts.min_apply_delay > 0) ||
+ (!IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) &&
sub->minapplydelay > 0))
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot enable %s mode for subscription with %s",
+    "streaming = parallel", "min_apply_delay"));
+

A. When can second condition ((!IsSet(opts.specified_opts,
SUBOPT_MIN_APPLY_DELAY) && sub->minapplydelay > 0)) in above check be
true?
B. In comments, you can say "See parse_subscription_options."

4.
+/*
+ * When min_apply_delay parameter is set on the subscriber, we wait long enough
+ * to make sure a transaction is applied at least that interval behind the
+ * publisher.

Shouldn't this part of the comment needs to be updated after the patch
has stopped using interval?

5. How does this feature interacts with the SKIP feature? Currently,
it doesn't care whether the changes of a particular xact are skipped
or not. I think that might be okay because anyway the purpose of this
feature is to make subscriber lag from publishers. What do you think?
I feel we can add some comments to indicate the same.

-- 
With Regards,
Amit Kapila.