On Fri, Dec 2, 2016 at 2:21 AM, Vladimir Rusinov <vrusi...@google.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
