Re: pg_xlogdump follow into the future

On Fri, Dec 2, 2016 at 2:21 AM, Vladimir Rusinov <vrusinov(at)google(dot)com>
wrote:

> 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
>

Patch received feedback at the end of commitfest.
Closed in 2016-11 commitfest with "moved to next CF".
Please feel free to update the status once you submit the updated patch.

Regards,
Hari Babu
Fujitsu Australia