Disallow negative values for max_retention_duration.

The subscription option max_retention_duration accepts an integer value
representing a timeout in milliseconds, where zero means unlimited
retention (no timeout).  Negative values have no useful meaning, but were
silently accepted and stored in the subscription catalog.

A negative value causes should_stop_conflict_info_retention() to always
return true, because TimestampDifferenceExceeds() treats a negative
threshold as already exceeded.  This stops dead tuple retention
immediately rather than honoring the configured timeout.

Fix by rejecting negative values for max_retention_duration during CREATE
SUBSCRIPTION and ALTER SUBSCRIPTION.

Author: Chao Li <lic@highgo.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/9232401A-DEEE-49E1-9D11-D14A776DB82B@gmail.com

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index fd026b304c24fa4ef7b258475222d6acd2b9b25c..87311f683e9bc5e519b3520b4827b3f6037dcfdf 100644 (file)
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -356,6 +356,11 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 
                        opts->specified_opts |= SUBOPT_MAX_RETENTION_DURATION;
                        opts->maxretention = defGetInt32(defel);
+
+                       if (opts->maxretention < 0)
+                               ereport(ERROR,
+                                               errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                               errmsg("max_retention_duration cannot be negative"));
                }
                else if (IsSet(supported_opts, SUBOPT_ORIGIN) &&
                                 strcmp(defel->defname, "origin") == 0)

diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 8481056a7023197eaa60f496c7f8fbe7f6a4bfdf..a1b3cc96d83a69acfc59f070b3c622aa3e24702a 100644 (file)
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -518,6 +518,9 @@ DROP SUBSCRIPTION regress_testsub;
 -- fail - max_retention_duration must be integer
 CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, max_retention_duration = foo);
 ERROR:  max_retention_duration requires an integer value
+-- fail - max_retention_duration must be non-negative
+CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, max_retention_duration = -1);
+ERROR:  max_retention_duration cannot be negative
 -- ok
 CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, max_retention_duration = 1000);
 NOTICE:  max_retention_duration is ineffective when retain_dead_tuples is disabled
@@ -530,6 +533,9 @@ HINT:  To initiate replication, you must manually create the replication slot, e
  regress_testsub | regress_subscription_user | f       | {testpub}   | f      | parallel  | d                | f                | any    | t                 | f             | f        |        | f                  |                   1000 | f                | off                | dbname=regress_doesnotexist | -1               | 0/00000000 | 
 (1 row)
 
+-- fail - max_retention_duration must be non-negative
+ALTER SUBSCRIPTION regress_testsub SET (max_retention_duration = -1);
+ERROR:  max_retention_duration cannot be negative
 -- ok
 ALTER SUBSCRIPTION regress_testsub SET (max_retention_duration = 0);
 \dRs+

diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 374fad6aa7b0ebb460255235dc03597c7b9fa934..528a10b5481c65da376a199c0a1e91e5984457f1 100644 (file)
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -378,11 +378,17 @@ DROP SUBSCRIPTION regress_testsub;
 -- fail - max_retention_duration must be integer
 CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, max_retention_duration = foo);
 
+-- fail - max_retention_duration must be non-negative
+CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, max_retention_duration = -1);
+
 -- ok
 CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, max_retention_duration = 1000);
 
 \dRs+
 
+-- fail - max_retention_duration must be non-negative
+ALTER SUBSCRIPTION regress_testsub SET (max_retention_duration = -1);
+
 -- ok
 ALTER SUBSCRIPTION regress_testsub SET (max_retention_duration = 0);