MIME-Version: 1.0
References: <f14463ab-990b-4ae9-a177-998d2677aae0@pgbackrest.org>
 <CAHGQGwEnakTrosMp3Y=Trya4MTC-h8Lqx3FDU1mOALRQ0rz59Q@mail.gmail.com>
 <9e8b7ee9-4a16-477a-baa5-0cdf37a04798@pgbackrest.org>
In-Reply-To: <9e8b7ee9-4a16-477a-baa5-0cdf37a04798@pgbackrest.org>
From: Fujii Masao <masao.fujii@gmail.com>
Date: Thu, 5 Mar 2026 00:41:01 +0900
Message-ID: 
 <CAHGQGwG44vZbSoBmg076G+xkR6n=Tj2=q+fVkfP7yEsyF1daFA@mail.gmail.com>
Subject: Re: Improve checks for GUC recovery_target_xid
To: David Steele <david@pgbackrest.org>
Cc: =?UTF-8?Q?H=C3=BCseyin_Demir?= <huseyin.d3r@gmail.com>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAHGQGwG44vZbSoBmg076G%2BxkR6n%3DTj2%3Dq%2BfVkfP7yEsyF1daFA%40mail.gmail.com>
Precedence: bulk

On Wed, Mar 4, 2026 at 2:11=E2=80=AFPM David Steele <david@pgbackrest.org> =
wrote:
>
> On 2/27/26 08:12, Fujii Masao wrote:
> > On Fri, Feb 20, 2026 at 2:42=E2=80=AFPM David Steele <david@pgbackrest.=
org> wrote:
> >
> > + GUC_check_errdetail("\"%s\" without epoch must greater than or equal =
to %u.",
> >
> > "must greater" shiould be "must be greater"?
>
> Fixed in v2 attached to the prior email.
>
> > "without epoch" seems not necessary to me.
>
> I guess that depends on whether or not we error if the epoch is present,
> see below.
>
> > + /*
> > + * This cast will remove the epoch, if any
> > + */
> > + xid =3D (TransactionId) strtou64(*newval, &endp, 0);
> >
> > Would it be better to use strtouint32_strict() instead of strtou64()?
> > That would allow us to detect invalid XID values larger than 2^32 and
> > report an error, similar to what pg_resetwal -x does.
>
> This was my first instinct, but it causes our integration tests to fail
> because pg_current_xact_id() returns the xid with epoch. You can fix
> this by casting pg_current_xact_id()::xid but this seems like a pretty
> big change in usage. I'm OK with it but we'd definitely need to update
> the documentation to match.
>
> What do you think?

I see your point, and I'm fine with the patch.

My earlier comment was based on my misunderstanding. I thought that only
32-bit transaction IDs were allowed for recovery_target_xid, and therefore
values larger than 2^32 should be rejected.

However, recovery_target_xid currently accepts both 32-bit XIDs and 64-bit
full transaction IDs (epoch + 32-bit XID). When a 64-bit value is specified=
,
only the 32-bit XID portion is used as the recovery target.
Given this behavior, your change seems to make sense.

Regarding the regression test, if the purpose is to verify the GUC hook
for recovery_target_xid, it might be simpler to test whether
"ALTER SYSTEM SET recovery_target_xid TO ..." succeeds or fails as expected
as follows, rather than starting the server with that setting. That said,
since recovery_target_timeline is already tested in a similar way, I unders=
tand
why you followed the same pattern here. So I'm ok with the current approach=
.

    my ($result, $stdout, $stderr) =3D $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)");

If we think it's better to use ALTER SYSTEM SET for testing invalid
recovery_target_xxx settings to keep the regression tests simpler,
we can revisit this later and address it in a separate patch.

Regards,

--=20
Fujii Masao