Re: pg_xlogdump follow into the future - Mailing list pgsql-hackers

This patch does not have a reviewer, so I've decided to try myself on.

Disclaimer: although I review quite a lot of code daily, this is my first review for PostgreSQL. I don't know code very well, and frankly I don't really know C very well.

Hope my effort are not vain and will be helpful to somebody.

I'll be happy for review on review and any tips on process.

Summary

=======

I favour this patch. Current behaviour is indeed confusing. If we keep current behaviour we need to update docs and maybe also print a warning when using -f with a file name.

Thank you for submission, but i'm afraid there is a bit more work here:

- There is a bug, making it hard to test. Please fix.

- Please add some tests.

Submission review

==============

Patch applies cleanly on HEAD. Tests succeed.

There are no new or affected by this patch tests. Given that I've found a trivial bug (see below), a test should be created.

Usability review

============

I believe I've immediately hit a corner case:

1) I've created a new instance, started it and run `./bin/pg_xlogdump -f db/pg_wal/000000010000000000000001`.

This spewed quite a lot of stuff, as expected.

2) I've connected to the same instance and ran following:

# SELECT pg_switch_xlog();

 pg_switch_xlog 

----------------

 0/14FA3D8

(1 row)

xlogdump immediately crashed with following:

pg_xlogdump: FATAL:  could not find file "000000010000000000000002": No such file or directory

Problem is that Postgres does not create file until it's actually needed. So 000000010000000000000002 indeed was not there until after I've run some transactions. I believe same would happen after pg_start_backup(), etc.

Feature review

===========

See above. Can't do more testing.

Performance review

===============

n/a

Coding review

===========

LGTM

Architecture review

==============

n/a