On 2025/06/05 3:46, Nikolay Samokhvalov wrote:
> Thank you, Peter and Michael, for the reviews
>
> Attached is v2, simplified as suggested:
> - Removed short option -s
> - Removed interactive confirmation and --force
> - Simplified tests leaving only essential ones
In my environment, the test failed with the following output:
# Failed test 'system identifier was changed correctly'
# at t/001_basic.pl line 203.
# got: '-8570200862721897295'
# expected: '9876543210987654321'
# Looks like you failed 1 test of 84.
t/001_basic.pl ......
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/84 subtests
I think this happens because the system_identifier returned
by pg_control_system() is a bigint, not an unsigned one.
So either we need to use a different method to retrieve
the value correctly, or we should fix pg_control_system() to
report the system identifier properly (though that would be
a separate issue).
+ if (set_sysid != 0)
+ {
+ printf(_("System identifier: " UINT64_FORMAT "\n"),
+ ControlFile.system_identifier);
+ }
For consistency with PrintControlValues() and pg_controldata,
the label should be "Database system identifier", and we should
use PRIu64 instead of UINT64_FORMAT for gettext()?
+ {"system-identifier", required_argument, NULL, 3},
{"next-transaction-id", required_argument, NULL, 'x'},
{"wal-segsize", required_argument, NULL, 1},
{"char-signedness", required_argument, NULL, 2},
It would be more consistent to place the "system-identifier"
entry just after "char-signedness".
One question about this feature: why do we need to allow
explicitly setting the system identifier? If the goal is
simply to ensure it's different from the original,
wouldn't it be sufficient to let pg_resetwal generate
a new (hopefully unique) value using existing logic,
like what's done in GuessControlValues() or BootStrapXLOG()?
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
