Improve validation of recovery_target_xid GUC values.

Previously, the recovery_target_xid GUC values were not sufficiently validated.
As a result, clearly invalid inputs such as the string "bogus", a decimal value
like "1.1", or 0 (a transaction ID smaller than the minimum valid value of 3)
were unexpectedly accepted. In these cases, the value was interpreted as
transaction ID 0, which could cause recovery to behave unexpectedly.

This commit improves validation of recovery_target_xid GUC so that invalid
values are rejected with an error. This prevents recovery from proceeding
with misconfigured recovery_target_xid settings.

Also this commit updates the documentation to clarify the allowed values
for recovery_target_xid GUC.

Author: David Steele <david@pgbackrest.org>
Reviewed-by: Hüseyin Demir <huseyin.d3r@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/f14463ab-990b-4ae9-a177-998d2677aae0@pgbackrest.org

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index cb7afca90798baa068b481f20cd964861aa3eebd..8cdd826fbd37a523e38e9d6c53b7e5496132d874 100644 (file)
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4334,6 +4334,21 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
         The precise stopping point is also influenced by
         <xref linkend="guc-recovery-target-inclusive"/>.
        </para>
+
+       <para>
+        The value can be specified as either a 32-bit transaction ID or a 64-bit
+        transaction ID (consisting of an epoch and a 32-bit ID), such as the
+        value returned by <function>pg_current_xact_id()</function>. When a
+        64-bit transaction ID is provided, only its 32-bit transaction ID
+        portion is used as the recovery target. For example, the values
+        4294968296 (epoch 1) and 8589935592 (epoch 2) both refer to the same
+        32-bit transaction ID, 1000.
+       </para>
+
+       <para>
+        The effective transaction ID (the 32-bit portion) must be greater than
+        or equal to 3.
+       </para>
       </listitem>
      </varlistentry>
 

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 31806dcf00889a948c5a8301ad31d3178fc65521..fbddd7e522c091d527a1fe46030b2fc069aaf5a3 100644 (file)
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -5044,11 +5044,38 @@ check_recovery_target_xid(char **newval, void **extra, GucSource source)
        {
                TransactionId xid;
                TransactionId *myextra;
+               char       *endp;
+               char       *val;
 
                errno = 0;
-               xid = (TransactionId) strtou64(*newval, NULL, 0);
-               if (errno == EINVAL || errno == ERANGE)
+
+               /*
+                * Consume leading whitespace to determine if number is negative
+                */
+               val = *newval;
+
+               while (isspace((unsigned char) *val))
+                       val++;
+
+               /*
+                * This cast will remove the epoch, if any
+                */
+               xid = (TransactionId) strtou64(val, &endp, 0);
+
+               if (*endp != '\0' || errno == EINVAL || errno == ERANGE || *val == '-')
+               {
+                       GUC_check_errdetail("\"%s\" is not a valid number.",
+                                                               "recovery_target_xid");
+                       return false;
+               }
+
+               if (xid < FirstNormalTransactionId)
+               {
+                       GUC_check_errdetail("\"%s\" without epoch must be greater than or equal to %u.",
+                                                               "recovery_target_xid",
+                                                               FirstNormalTransactionId);
                        return false;
+               }
 
                myextra = (TransactionId *) guc_malloc(LOG, sizeof(TransactionId));
                if (!myextra)

diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index e0df1a23423bad247d3c02ec89d2dec7196cba91..4e36e3a3fb5a6a2f790fb22db2f358f1bddd6413 100644 (file)
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -240,4 +240,26 @@ ok(!$res, 'invalid timeline target (upper bound check)');
 $log_start =
   $node_standby->wait_for_log("must be between 1 and 4294967295", $log_start);
 
+# Invalid recovery_target_xid tests
+my ($result, $stdout, $stderr) = $node_primary->psql('postgres',
+       "ALTER SYSTEM SET recovery_target_xid TO 'bogus'");
+like(
+       $stderr,
+       qr/is not a valid number/,
+       "invalid recovery_target_xid (bogus value)");
+
+($result, $stdout, $stderr) = $node_primary->psql('postgres',
+       "ALTER SYSTEM SET recovery_target_xid TO '-1'");
+like(
+       $stderr,
+       qr/is not a valid number/,
+       "invalid recovery_target_xid (negative)");
+
+($result, $stdout, $stderr) = $node_primary->psql('postgres',
+       "ALTER SYSTEM SET recovery_target_xid TO '0'");
+like(
+       $stderr,
+       qr/without epoch must be greater than or equal to 3/,
+       "invalid recovery_target_xid (lower bound check)");
+
 done_testing();