Обсуждение: Re: Add --system-identifier / -s option to pg_resetwal

On 31.05.25 20:52, Nikolay Samokhvalov wrote:
> the attached patch adds a new -s / --system-identifier option to 
> pg_resetwal that allows users to change the database cluster's system 
> identifier; it can be useful when you need to make a restored cluster 
> distinct from the original, or when cloning for testing

Maybe we can stop eating up short options?  A long option seems 
sufficient for this.

> - requires interactive confirmation or --force flag for safety

I don't see the need for interactive confirmation here.  The user would 
have explicitly chosen the option, so they get what they asked for.

On 04.06.25 20:46, Nikolay Samokhvalov wrote:
> - Simplified logic, getting rid of '-' check (negative numbers) -- 
> decided to accept negative input values (they wrap to valid positive uint64)

That doesn't seem like a good idea to me.

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

On Sun, Feb 1, 2026 at 6:33 AM Nikolay Samokhvalov <nik@postgres.ai> wrote:
>
> Thank you for the review!
>
> Attached is v3 addressing your feedback:

Thanks for updating the patch!

With this patch, the test failed on my laptop as follows.
Could you please take a look and fix it?

$ make -s check -C src/bin/pg_resetwal/ PROVE_TESTS="t/001*"
# +++ tap check in src/bin/pg_resetwal +++
t/001_basic.pl .. 15/? # Tests were run but no plan was declared and
done_testing() was not seen.
# Looks like your test exited with 4 just after 82.
t/001_basic.pl .. Dubious, test returned 4 (wstat 1024, 0x400)
All 82 subtests passed

Test Summary Report
-------------------
t/001_basic.pl (Wstat: 1024 Tests: 82 Failed: 0)
  Non-zero exit status: 4
  Parse errors: No plan found in TAP output
Files=1, Tests=82,  4 wallclock secs ( 0.03 usr  0.01 sys +  0.31 cusr
 0.65 csys =  1.00 CPU)
Result: FAIL
make: *** [check] Error 1

Regards,

--
Fujii Masao