pg_xlogdump follow into the future

On 14 July 2016 at 07:46, Magnus Hagander <magnus@hagander.net> 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 seems like an oversight - if you specify -f with no end position, it
should follow "into the future" for any new files that appear. This allows
us to track what's happening properly.

AFAICT the actual code tracks this just fine, but the option parsing
prevents this from happening as it does not allow the end pointer to be
unset. Attach patch fixes this and makes it follow "forever" if you specify
-f without an end pointer.

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

Good thinking.

Patch looks good, but not tested. Needs comment on the second chunk.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

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.

diff --git a/src/bin/pg_xlogdump/pg_xlogdump.c b/src/bin/pg_xlogdump/pg_xlogdump.c
index c0a6816..2f1018b 100644
--- a/src/bin/pg_xlogdump/pg_xlogdump.c
+++ b/src/bin/pg_xlogdump/pg_xlogdump.c
@@ -901,8 +901,14 @@ main(int argc, char **argv)
goto bad_argument;
}

-		/* no second file specified, set end position */
-		if (!(optind + 1 < argc) && XLogRecPtrIsInvalid(private.endptr))
+		/*
+		 * No second file specified, so unless we are in follow mode,
+		 * set the end position to the end of the same segment as
+		 * the start position.
+		 */
+		if (!(optind + 1 < argc) &&
+			XLogRecPtrIsInvalid(private.endptr) &&
+			!config.follow)
XLogSegNoOffsetToRecPtr(segno + 1, 0, private.endptr);

/* parse ENDSEG if passed */
@@ -933,7 +939,8 @@ main(int argc, char **argv)
}

-		if (!XLByteInSeg(private.endptr, segno) &&
+		if (!XLogRecPtrIsInvalid(private.endptr) &&
+			!XLByteInSeg(private.endptr, segno) &&
private.endptr != (segno + 1) * XLogSegSize)
{
fprintf(stderr,

Other than that it looks reasonable from a quick glance.

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jul 14, 2016 at 8:27 PM, Andres Freund <andres@anarazel.de> 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 :)

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.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Mon, Jul 18, 2016 at 8:10 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Thu, Jul 14, 2016 at 8:27 PM, Andres Freund <andres@anarazel.de> 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/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

--
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 <michael.paquier@gmail.com>
wrote:

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

On Fri, Dec 2, 2016 at 1:36 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:

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.

And the thread has died as well weeks ago. I am marking that as
"returned with feedback" as there are no new patches.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers