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

On Tue, 10 Jan 2023 at 19:41, Takamichi Osumi (Fujitsu)
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Tuesday, January 3, 2023 4:01 PM vignesh C <vignesh21(at)gmail(dot)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