Re: BUG #16780: Inconsistent recovery_target_xid handling across platforms

On Fri, Dec 18, 2020 at 10:00:00AM +0000, PG Bug reporting form wrote:
> So on Windows the recovery_target_xid parameter doesn't accept 64-bit
> value.
> As I see in check_recovery_target_xid() the strtoul() function is used to
> check/convert the input value, that is later stored into 32-bit variable.
> Thus, on platforms where unsigned long is 32-bit wide, you can't specify a
> result of pg_current_xact_id() as recovery_target_xid, while on platforms
> where unsigned long is 64-bit, you can, but high 32 bits are just ignored.

In order to make the experience consistent across all platforms, we
can use pg_strtouint64() for the GUC parsing.  I agree that ignoring
the high bits can be confusing as recovery would stop once a XID
matches with a modulo of UINT32_MAX, but beginning to issue an error
on platforms where unsigned long is 8 bytes could also break existing
tools.  At the same time, WAL record headers and 2PC state data only
use 32-bit XIDs, so ignoring the high bits changes nothing at
recovery (XIDs don't go across more than one epoch in the WAL stream
AFAIK).  In short, I agree that there is room for improvement here and
that we should just allow the case to work by replacing the use of
strtoul() to pg_strtouint64().  I am ready to bet that a lot of users
setting a target XID rely on the return result of txid_current() or
pg_current_xact_id() similarly to the test failing here.  And once the
epoch increments, the problems begin.

Thoughts?
--
Michael