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

On Tue, 10 Jan 2023 at 19:41, Takamichi Osumi (Fujitsu)
<osumi.takamichi@fujitsu.com> wrote:
>
> On Tuesday, January 3, 2023 4:01 PM vignesh C <vignesh21@gmail.com> wrote:
> Hi, thanks for your review !
>
> Please have a look at the updated patch.

Thanks for the updated patch, few comments:
1) Comment inconsistency across create and alter subscription, better
to keep it same:
+       /*
+        * Do additional checking for disallowed combination when
min_apply_delay
+        * was not zero.
+        */
+       if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
+               opts->min_apply_delay > 0)
+       {
+               if (opts->streaming == LOGICALREP_STREAM_PARALLEL)
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_SYNTAX_ERROR)),
+                                       errmsg("min_apply_delay must
not be set when streaming = parallel"));
+       }

+                                       /*
+                                        * Test the combination of
streaming mode and
+                                        * min_apply_delay
+                                        */
+                                       if (opts.streaming ==
LOGICALREP_STREAM_PARALLEL &&
+                                               sub->minapplydelay > 0)
+                                               ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+
errmsg("min_apply_delay must not be set when streaming = parallel")));

2) ereport inconsistency, braces around errcode is present in few
places and not present in few places, it is better to keep it
consistent by removing it:
2.a)
+               if (opts->streaming == LOGICALREP_STREAM_PARALLEL)
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_SYNTAX_ERROR)),
+                                       errmsg("min_apply_delay must
not be set when streaming = parallel"));

2.b)
+                                       if (opts.streaming ==
LOGICALREP_STREAM_PARALLEL &&
+                                               sub->minapplydelay > 0)
+                                               ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+
errmsg("min_apply_delay must not be set when streaming = parallel")));

2.c)
+                                       if (opts.min_apply_delay > 0 &&
+                                               sub->stream ==
LOGICALREP_STREAM_PARALLEL)
+                                               ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+
errmsg("min_apply_delay must not be set when streaming = parallel")));

2.d)
+       if (pg_mul_s64_overflow(days, MSECS_PER_DAY, &result))
+               ereport(ERROR,
+                               (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+                                errmsg("bigint out of range")));

2.e)
+       if (pg_add_s64_overflow(result, ms, &result))
+               ereport(ERROR,
+                               (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+                                errmsg("bigint out of range")));

3) this include is not required, I could compile without it
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -48,6 +48,7 @@
 #include "utils/memutils.h"
 #include "utils/pg_lsn.h"
 #include "utils/syscache.h"
+#include "utils/timestamp.h"

4)
4.a)
Should this be changed:
/* Adds portion time (in ms) to the previous result. */
to
/* Adds portion time (in ms) to the previous result */

4.b)
Should this be changed:
/* Detect whether the value of interval can cause an overflow. */
to
/* Detect whether the value of interval can cause an overflow */

5) Can this "ALTER SUBSCRIPTION regress_testsub SET (min_apply_delay =
'1d')" be combined along with "-- success -- 123 ms", that way few
statements could be reduced
+-- success -- 86400000 ms
+CREATE SUBSCRIPTION regress_testsub CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect =
false, min_apply_delay = 123);
+ALTER SUBSCRIPTION regress_testsub SET (min_apply_delay = '1d');
+
+\dRs+
+
+ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE);
+DROP SUBSCRIPTION regress_testsub;

6) Can we do the interval testing along with alter subscription and
combined with "-- success -- 123 ms" test, that way few statements
could be reduced
+-- success -- interval is converted into ms and stored as integer
+CREATE SUBSCRIPTION regress_testsub CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect =
false, min_apply_delay = '4h 27min 35s');
+
+\dRs+
+
+ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE);
+DROP SUBSCRIPTION regress_testsub;

Regards,
Vignesh