Hi, > I noticed that pg_resetwal has poor test coverage. There are some TAP > tests, but they all run with -n, so they don't actually test the full > functionality. (There is a non-dry-run call of pg_resetwal in the > recovery test suite, but that is incidental.) > > So I added a bunch of more tests to test all the different options and > scenarios. That also led to adding more documentation about more > details how some of the options behave, and some tweaks in the program > output. > > It's attached as one big patch, but it could be split into smaller > pieces, depending what gets agreement.
All in all the patch looks OK but I have a couple of nitpicks. ``` + working on a data directory in an unclean shutdown state or with a corrupt + control file. ``` ``` + After running this command on a data directory with corrupted WAL or a + corrupt control file, ``` I'm not a native English speaker but shouldn't it be "corruptED control file"? ``` + Force <command>pg_resetwal</command> to proceed even in situations where + it could be dangerous, ``` "where" is probably fine but wouldn't "WHEN it could be dangerous" be better? ``` + // FIXME: why 2? if (set_oldest_commit_ts_xid < 2 && ``` Should we rewrite this to < FrozenTransactionId ? ``` +$mult = 32 * $blcksz * 4; # FIXME ``` Unless I'm missing something this $mult value is not used. Is it really needed here? -- Best regards, Aleksander Alekseev