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.


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
This spewed quite a lot of stuff, as expected.

2) I've connected to the same instance and ran following:
# SELECT pg_switch_xlog();
(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

Coding review

Architecture review

Vladimir Rusinov
Bigtable SRE

Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland
Registered in Dublin, Ireland
Registration Number: 368047

On Mon, Oct 3, 2016 at 5:44 AM, Michael Paquier <>

> On Mon, Jul 18, 2016 at 8:10 PM, Magnus Hagander <>
> wrote:
> >
> >
> > On Thu, Jul 14, 2016 at 8:27 PM, Andres Freund <>
> wrote:
> >>
> >> On 2016-07-14 13:46:23 +0200, Magnus Hagander wrote:
> >> > Currently, if you run pg_xlogdump with -f, you have to specify an end
> >> > position in an existing file, or if you don't it will only follow
> until
> >> > the
> >> > end of the current file.
> >>
> >> That's because specifying a file explicitly says that you only want to
> >> look at that file, specifying two files that you want the range
> >> inclusively between the two files.  -f works if you just use -s.
> >
> >
> > Hmm. It does now. I'm *sure* it didn't when I was testing it. It must've
> > been something else that was broken at that point :)
> Same as Andres here, my understanding is that one file means that you
> just want to look at this file, and two files permits to look at a
> range of changes. But I have no argument against changing the current
> behavior either.
> >> > I'd appreciate a review of that by someone who's done more work on the
> >> > xlog
> >> > stuff, but it seems trivial to me. Not sure I can argue it's a bugfix
> >> > though, since the usecase simply did not work...
> >>
> >> I'd say it's working as intended, and you want to change that
> >> intent. That's fair, but I'd not call it a bug, and I'd say it's not
> >> really 9.6 material.
> >
> > Based on that, I agree that it's working as intended.
> >
> > And definitely that it's not 9.6 material.
> >
> > I'll stick it on the CF page so I don't forget about it.
> Moved to next CF. Magnus, you may want to finish wrapping that if you
> still intend to change the current behavior.
> --
> Michael
> --
> Sent via pgsql-hackers mailing list (
> To make changes to your subscription:

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature

Reply via email to