Re: [HACKERS] pg_xlogdump follow into the future

[Michael Paquier]

On Fri, Dec 2, 2016 at 1:36 PM, Haribabu Kommi  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

Re: [HACKERS] pg_xlogdump follow into the future

[Haribabu Kommi]

On Fri, Dec 2, 2016 at 2:21 AM, Vladimir Rusinov 
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/00010001`.
> 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 "00010002": No
> such file or directory
>
> Problem is that Postgres does not create file until it's actually needed.
> So 00010002 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

Re: [HACKERS] pg_xlogdump follow into the future

[Vladimir Rusinov]

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/00010001`.
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 "00010002": No
such file or directory

Problem is that Postgres does not create file until it's actually needed.
So 00010002 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 
wrote:

> 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 (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [HACKERS] pg_xlogdump follow into the future

[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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Fujii Masao]

On Fri, Aug 26, 2016 at 10:03 PM, Fujii Masao  wrote:
> On Wed, Mar 23, 2016 at 7:04 PM, Pavan Deolasee
>  wrote:
>>
>>
>> On Wed, Mar 23, 2016 at 1:13 PM, Michael Paquier 
>> wrote:
>>>
>>>
>>> +   /*
>>> +* Compute targetRecOff. It should typically be greater than short
>>> +* page-header since a valid record can't , but can also be zero
>>> when
>>> +* caller has supplied a page-aligned address or when we are
>>> skipping
>>> +* multi-page continuation record. It doesn't matter though
>>> because
>>> +* ReadPageInternal() will read at least short page-header worth
>>> of
>>> +* data
>>> +*/
>>> This needs some polishing, there is an unfinished sentence here.
>>>
>>> +   targetRecOff = tmpRecPtr % XLOG_BLCKSZ;
>>> targetRecOff, pageHeaderSize and targetPagePtr could be declared
>>> inside directly the new while loop.
>>
>>
>> Thanks Michael for reviewing the patch. I've fixed these issues and new
>> version is attached.
>
> The patch looks good to me. Barring any objections,
> I'll push this and back-patch to 9.3 where pg_xlogdump was added.

Pushed. Thanks!

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Fujii Masao]

On Wed, Mar 23, 2016 at 7:04 PM, Pavan Deolasee
 wrote:
>
>
> On Wed, Mar 23, 2016 at 1:13 PM, Michael Paquier 
> wrote:
>>
>>
>> +   /*
>> +* Compute targetRecOff. It should typically be greater than short
>> +* page-header since a valid record can't , but can also be zero
>> when
>> +* caller has supplied a page-aligned address or when we are
>> skipping
>> +* multi-page continuation record. It doesn't matter though
>> because
>> +* ReadPageInternal() will read at least short page-header worth
>> of
>> +* data
>> +*/
>> This needs some polishing, there is an unfinished sentence here.
>>
>> +   targetRecOff = tmpRecPtr % XLOG_BLCKSZ;
>> targetRecOff, pageHeaderSize and targetPagePtr could be declared
>> inside directly the new while loop.
>
>
> Thanks Michael for reviewing the patch. I've fixed these issues and new
> version is attached.

The patch looks good to me. Barring any objections,
I'll push this and back-patch to 9.3 where pg_xlogdump was added.

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] pg_xlogdump follow into the future

[Magnus Hagander]

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



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

Re: [HACKERS] pg_xlogdump follow into the future

[Andres Freund]

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

Re: [HACKERS] pg_xlogdump follow into the future

[Simon Riggs]

On 14 July 2016 at 07:46, 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 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 Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] pg_xlogdump bad error msg?

[Magnus Hagander]

On Mon, Jul 11, 2016 at 5:20 PM, Andres Freund  wrote:

> On 2016-07-11 13:36:37 +0200, Magnus Hagander wrote:
> > When you don't specify a start segment to pg_xlogdump, you get:
> >
> > pg_xlogdump: no start log position given in range mode.
> >
> >
> > What is "range mode", and is there any other mode for pg_xlogdump? Should
> > it not just be "no start log position or segment given" or something like
> > that?
>
> Works for me as well.
>

I assume that means you agree with changing it, so I will.

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

[HACKERS] pg_xlogdump follow into the future

[Magnus Hagander]

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

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
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,

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

Re: [HACKERS] pg_xlogdump bad error msg?

[Andres Freund]

On 2016-07-11 13:36:37 +0200, Magnus Hagander wrote:
> When you don't specify a start segment to pg_xlogdump, you get:
> 
> pg_xlogdump: no start log position given in range mode.
> 
> 
> What is "range mode", and is there any other mode for pg_xlogdump? Should
> it not just be "no start log position or segment given" or something like
> that?

Works for me as well.

Andres


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

[HACKERS] pg_xlogdump bad error msg?

[Magnus Hagander]

When you don't specify a start segment to pg_xlogdump, you get:

pg_xlogdump: no start log position given in range mode.


What is "range mode", and is there any other mode for pg_xlogdump? Should
it not just be "no start log position or segment given" or something like
that?

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

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Craig Ringer]

On 1 April 2016 at 21:30, Craig Ringer  wrote:


> I'll attach the new testcase once I either get it to reproduce this bug or
> give up and leave the basic xlogdump testcase alone.
>

I had another bash at this and I still can't reproduce it on master using
the giant commit record approach Andres suggested. In fact I generated a
commit record larger than an entire xlog segment and it was still fine.

The DO procedure I posted upthread, when run on 9.4, reliably produces
segments that the xlogreader cannot decode with the symptoms Pavan
reported. It's fine on 9.6.

So I can't reproduce this on 9.6, but there might be a separate bug on 9.4.

I've attached a patch with the simple tests I added for pg_xlogdump as part
of this. I doubt it'd be desirable to commit the ridiculous commit record
part, but that's trivially removed, and I left it in place in case someone
else wanted to fiddle with other ways to reproduce this.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From bf05b552dd9eb5d2e9b50c77928426817033685d Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Wed, 6 Apr 2016 13:57:32 +0800
Subject: [PATCH] Tests for pg_xlogdump

---
 src/bin/pg_xlogdump/Makefile |   6 +
 src/bin/pg_xlogdump/t/010_pg_xlogdump.pl | 249 +++
 2 files changed, 255 insertions(+)
 create mode 100644 src/bin/pg_xlogdump/t/010_pg_xlogdump.pl

diff --git a/src/bin/pg_xlogdump/Makefile b/src/bin/pg_xlogdump/Makefile
index 11df47d..c1138d2 100644
--- a/src/bin/pg_xlogdump/Makefile
+++ b/src/bin/pg_xlogdump/Makefile
@@ -38,3 +38,9 @@ uninstall:
 
 clean distclean maintainer-clean:
 	rm -f pg_xlogdump$(X) $(OBJS) $(RMGRDESCSOURCES) xlogreader.c
+
+check:
+	$(prove_check)
+
+installcheck:
+	$(prove_installcheck)
diff --git a/src/bin/pg_xlogdump/t/010_pg_xlogdump.pl b/src/bin/pg_xlogdump/t/010_pg_xlogdump.pl
new file mode 100644
index 000..93b2063
--- /dev/null
+++ b/src/bin/pg_xlogdump/t/010_pg_xlogdump.pl
@@ -0,0 +1,249 @@
+use strict;
+use warnings;
+use bigint qw(hex);
+use Cwd;
+use Config;
+use File::Basename qw(basename);
+use List::Util qw(minstr maxstr);
+use PostgresNode;
+use TestLib;
+use Test::More;
+use Data::Dumper;
+
+my $verbose = 1;
+
+sub slurp_xlogdump_lines
+{
+	my ($node, $timeline, $firstwal, $lastwal) = @_;
+	my ($stdout, $stderr) = ('', '');
+
+	if (defined($firstwal) && $firstwal =~ /^[[:xdigit:]]{24}$/)
+	{
+		$firstwal = $node->data_dir . "/pg_xlog/" . $firstwal;
+	}
+
+	if (defined($lastwal) && $lastwal !~ /^[[:xdigit:]]{24}$/)
+	{
+		die("pg_xlogdump expects the last WAL seg to be a bare filename, not '$lastwal'");
+	}
+
+	if (!defined($firstwal) || !defined($lastwal))
+	{
+		my $wal_pattern = sprintf("%s/pg_xlog/%08X" . ("?" x 16), $node->data_dir, $timeline);
+		my @wal = glob $wal_pattern;
+		$firstwal = List::Util::minstr(@wal) if !defined($firstwal);
+		$lastwal = basename(List::Util::maxstr(@wal)) if !defined($lastwal);
+	}
+
+	diag("decoding from " . $firstwal . " to " . $lastwal)
+		if $verbose;
+
+	IPC::Run::run ['pg_xlogdump', $firstwal, $lastwal], '>', \$stdout, '2>', \$stderr;
+
+	like($stderr, qr/(?:invalid record length at [0-9A-F]+\/[0-9A-F]+: wanted 24, got 0|^\s*$)/,
+		'pg_xlogdump exits with expected error or silence');
+
+	diag "xlogdump exited with: '$stderr'"
+		if $verbose;
+
+	my $lineno = 1;
+
+	my @xld_lines = split("\n", $stdout);
+
+	return \@xld_lines;
+}
+
+sub match_xlogdump_lines
+{
+	my ($node, $timeline, $firstwal, $lastwal) = @_;
+
+	my $xld_lines = slurp_xlogdump_lines($node, $timeline, $firstwal, $lastwal);
+
+	my @records;
+	my $lineno = 1;
+
+	for my $xld_line (@$xld_lines)
+	{
+		chomp $xld_line;
+
+		# We don't use Test::More's 'like' here because it'd run a variable number
+		# of tests. Instead do our own diagnostics and fail.
+		if ($xld_line =~ /^rmgr: (\w+)\s+len \(rec\/tot\):\s*([[:digit:]]+)\/\s*([[:digit:]]+), tx:\s*([[:digit:]]*), lsn: ([[:xdigit:]]{1,8})\/([[:xdigit:]]{1,8}), prev ([[:xdigit:]]{1,8})\/([[:xdigit:]]{1,8}), desc: (.*)$/)
+		{
+			my %record = (
+'_raw' => length($xld_line) >= 100 ? substr($xld_line,0,100) . "..." : $xld_line,
+'rmgr' => $1,
+'len_rec' => int($2),
+'len_tot' => int($3),
+'tx' => defined($4) ? int($4) : undef,
+'lsn' => (hex($5)<<32) + hex($6),
+'prev' => (hex($7)<<32) + hex($8),
+'lsn_str' => "$5/$6",
+'prev_str' => "$7/$8",
+'desc' => length($9) >= 100 ? substr($9,0,100) . "..." : $9,
+			);
+
+			if ($record{'prev'} >= $record{'lsn'})
+			{
+diag("in xlog record on line $lineno:\n$xld_line\n   ... prev ptr $record{prev_str} is greater than or equal to lsn ptr $record{lsn_str}");
+cmp_ok($record{prev}, 'lt', $record{lsn}, 'previous pointer less than lsn ptr');
+			}
+
+			push @records, \%record;
+		}
+		else
+		{
+			diag("xlog record on line $lineno:\n$xld_line\n   ...does not match the test pattern");
+			fail("an xlog record didn't match the expected p

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Craig Ringer]

On 31 March 2016 at 16:13, Andres Freund  wrote:


> It's probably easier to just generate a humongous commit record. You can
> do so by having a *lot* of subtransactions. Relatively easy to do with
> plpgsql by creating them in a loop (SELECT txid_current() in EXCEPTION
> bearing block ought to suffice).
>
>
This does the trick and does it quickly on 9.4:

CREATE TABLE trivial(padding text);
ALTER TABLE trivial ALTER COLUMN padding SET STORAGE plain;

DO
LANGUAGE plpgsql
$$
DECLARE
  wal_seg_size integer;
  remaining_size integer;
BEGIN
  wal_seg_size := (select setting from pg_settings where name =
'wal_segment_size')::integer
* (select setting from pg_settings where name =
'wal_block_size')::integer;
  LOOP
SELECT wal_seg_size - file_offset FROM
pg_xlogfile_name_offset(pg_current_xlog_insert_location()) INTO
remaining_size;
IF remaining_size < 8192
THEN
-- Make a nice big commit record right at the end of the segment
EXIT;
ELSE
BEGIN
  -- About 4k
  INSERT INTO trivial(padding) VALUES (repeat('0123456789abcdef',
256));
EXCEPTION
  WHEN division_by_zero THEN
CONTINUE;
END;
END IF;
  END LOOP;
END;
$$;


I had no issue reproducing the bug on 9.4, but I don't see it in 9.6. At
least, not this one, not yet.

Still, we might want to backpatch the fix; it seems clear enough even if I
can't reproduce the issue yet. It doesn't look like it'll affect logical
decoding since the snapshot builder has its own unrelated logic for finding
the startpoint for decoding, and it certainly doesn't affect WAL replay
otherwise we would've seen the fireworks much earlier. The upside is that
also makes the backpatch much safer.

I was surprised to see that there are no tests for pg_xlogdump to add this
on to, so I've added a trivial test. I've got some more complete
xlogdump-helper code in the failover slots tests that I should extract and
add to PostgresNode.pm but this'll do in the mean time and is much simpler.

In the process I noticed that pg_xlogdump doesn't follow the rules of the
rest of the codebase when it comes to command line handling with --version,
returning nonzero on bad options, etc. It might be good to fix that;
there's a small BC impact, but I doubt anyone cares when it comes to
pg_xlogdump.

I'll attach the new testcase once I either get it to reproduce this bug or
give up and leave the basic xlogdump testcase alone.


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Andres Freund]

On 2016-03-31 09:41:46 +0530, Pavan Deolasee wrote:
> On Thu, Mar 31, 2016 at 6:27 AM, Craig Ringer  wrote:
> >
> >
> > Can you describe the process used to generate the sample WAL segment?
> >
> >
> Shame that I can't find the sql file used to create the problematic WAL
> segment. But this is what I did.

It's probably easier to just generate a humongous commit record. You can
do so by having a *lot* of subtransactions. Relatively easy to do with
plpgsql by creating them in a loop (SELECT txid_current() in EXCEPTION
bearing block ought to suffice).

Greetings,

Andres Freund


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

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Pavan Deolasee]

On Thu, Mar 31, 2016 at 6:27 AM, Craig Ringer  wrote:
>
>
> Can you describe the process used to generate the sample WAL segment?
>
>
Shame that I can't find the sql file used to create the problematic WAL
segment. But this is what I did.

I wrote a plpgsql function which inserts rows in a loop, every time
checking if the remaining space in the WAL segment  has fallen to less than
couple of hundred bytes. Of course, I used pg_settings to get the WAL
segment size, WAL page size and pg_current_xlog_insert_location() to
correctly compute remaining bytes in the WAL segment. At this point, do a
non-HOT update, preferably to table which is already fully vacuumed and
CHECKPOINTed to avoid getting any other WAL records in between. Assuming
FPW is turned ON, the UPDATE should generate enough WAL to cross over the
first page in the new WAL segment.

Let me know if you would like to me to put together a sql based on this
description.

Thanks,
Pavan

-- 
 Pavan Deolasee   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Craig Ringer]

On 23 March 2016 at 18:04, Pavan Deolasee  wrote:

>
>
> On Wed, Mar 23, 2016 at 1:13 PM, Michael Paquier <
> michael.paqu...@gmail.com> wrote:
>
>>
>> +   /*
>> +* Compute targetRecOff. It should typically be greater than short
>> +* page-header since a valid record can't , but can also be zero
>> when
>> +* caller has supplied a page-aligned address or when we are
>> skipping
>> +* multi-page continuation record. It doesn't matter though
>> because
>> +* ReadPageInternal() will read at least short page-header worth
>> of
>> +* data
>> +*/
>> This needs some polishing, there is an unfinished sentence here.
>>
>> +   targetRecOff = tmpRecPtr % XLOG_BLCKSZ;
>> targetRecOff, pageHeaderSize and targetPagePtr could be declared
>> inside directly the new while loop.
>>
>
> Thanks Michael for reviewing the patch. I've fixed these issues and new
> version is attached.
>

Looks sensible to me based on a reading of "git diff -w" of the applied
patch. It passes make check and make -C src/test/recovery check . Marked
ready for committer; I'd like to add a TAP test for it, but it's ready to
go without that.



-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Craig Ringer]

On 23 March 2016 at 18:04, Pavan Deolasee  wrote:

>
>
> On Wed, Mar 23, 2016 at 1:13 PM, Michael Paquier <
> michael.paqu...@gmail.com> wrote:
>
>>
>> +   /*
>> +* Compute targetRecOff. It should typically be greater than short
>> +* page-header since a valid record can't , but can also be zero
>> when
>> +* caller has supplied a page-aligned address or when we are
>> skipping
>> +* multi-page continuation record. It doesn't matter though
>> because
>> +* ReadPageInternal() will read at least short page-header worth
>> of
>> +* data
>> +*/
>> This needs some polishing, there is an unfinished sentence here.
>>
>> +   targetRecOff = tmpRecPtr % XLOG_BLCKSZ;
>> targetRecOff, pageHeaderSize and targetPagePtr could be declared
>> inside directly the new while loop.
>>
>
> Thanks Michael for reviewing the patch. I've fixed these issues and new
> version is attached.
>
>
Can you describe the process used to generate the sample WAL segment?

I'd like to turn it into a TAP test to go along with the patch.



-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Michael Paquier]

On Mon, Mar 28, 2016 at 10:15 PM, Andres Freund  wrote:
> It's definitely too late for that; they're going to be wrapped in a
> couple hours.

I have added this patch to the next CF so as we do not lose track of this bug:
https://commitfest.postgresql.org/10/593/
-- 
Michael


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

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Andres Freund]

On 2016-03-28 13:21:41 +0530, Pavan Deolasee wrote:
> On Wed, Mar 23, 2016 at 6:16 PM, Michael Paquier 
> wrote:
> 
> >
> >
> > I'd just add dots at the end of the sentences in the comment blocks
> > because that's project style, but I'm being picky, except that the
> > logic looks quite good.
> >
> 
> Since this is a bug affecting all stable branches, IMHO it will be a good
> idea to fix this before the upcoming minor releases.

It's definitely too late for that; they're going to be wrapped in a
couple hours.

Andres


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

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Pavan Deolasee]

On Wed, Mar 23, 2016 at 6:16 PM, Michael Paquier 
wrote:

>
>
> I'd just add dots at the end of the sentences in the comment blocks
> because that's project style, but I'm being picky, except that the
> logic looks quite good.
>

Since this is a bug affecting all stable branches, IMHO it will be a good
idea to fix this before the upcoming minor releases.

Thanks,
Pavan

-- 
 Pavan Deolasee   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Michael Paquier]

On Wed, Mar 23, 2016 at 7:04 PM, Pavan Deolasee
 wrote:
>
>
> On Wed, Mar 23, 2016 at 1:13 PM, Michael Paquier 
> wrote:
>>
>>
>> +   /*
>> +* Compute targetRecOff. It should typically be greater than short
>> +* page-header since a valid record can't , but can also be zero
>> when
>> +* caller has supplied a page-aligned address or when we are
>> skipping
>> +* multi-page continuation record. It doesn't matter though
>> because
>> +* ReadPageInternal() will read at least short page-header worth
>> of
>> +* data
>> +*/
>> This needs some polishing, there is an unfinished sentence here.
>>
>> +   targetRecOff = tmpRecPtr % XLOG_BLCKSZ;
>> targetRecOff, pageHeaderSize and targetPagePtr could be declared
>> inside directly the new while loop.
>
>
> Thanks Michael for reviewing the patch. I've fixed these issues and new
> version is attached.

I'd just add dots at the end of the sentences in the comment blocks
because that's project style, but I'm being picky, except that the
logic looks quite good.
-- 
Michael


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

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Pavan Deolasee]

On Wed, Mar 23, 2016 at 1:13 PM, Michael Paquier 
wrote:

>
> +   /*
> +* Compute targetRecOff. It should typically be greater than short
> +* page-header since a valid record can't , but can also be zero
> when
> +* caller has supplied a page-aligned address or when we are
> skipping
> +* multi-page continuation record. It doesn't matter though because
> +* ReadPageInternal() will read at least short page-header worth of
> +* data
> +*/
> This needs some polishing, there is an unfinished sentence here.
>
> +   targetRecOff = tmpRecPtr % XLOG_BLCKSZ;
> targetRecOff, pageHeaderSize and targetPagePtr could be declared
> inside directly the new while loop.
>

Thanks Michael for reviewing the patch. I've fixed these issues and new
version is attached.

Thanks,
Pavan

-- 
 Pavan Deolasee   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


xlogdump_multipage_cont_record_v2.patch
Description: Binary data

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

Re: [HACKERS] pg_xlogdump fails to handle WAL file with multi-page XLP_FIRST_IS_CONTRECORD data

[Michael Paquier]

On Wed, Mar 23, 2016 at 3:43 PM, Pavan Deolasee
 wrote:
> While investigating some issue, I found that pg_xlogdump fails to dump
> contents from a WAL file if the file has continuation data from previous WAL
> record and the data spans more than one page. In such cases,
> XLogFindNextRecord() fails to take into account that there will be more than
> one xlog page headers (long and short) and thus tries to read from an offset
> where no valid record exists. That results in pg_xlogdump throwing error
> such as:

OK. That makes sense, that is indeed a possible scenario.

> Attached WAL file from master branch demonstrates the issue, generated using
> synthetic data. Also, attached patch fixes it for me.

So does it for me.

> While we could have deduced the number of short and long headers and skipped
> directly to the offset, I found reading one page at a time and using
> XLogPageHeaderSize() to find header size of each page separately, a much
> cleaner way. Also, the continuation data is not going to span many pages. So
> I don't see any harm in doing it that way.

I have to agree, the new code is pretty clean this way by calculating
the next page LSN directly in the loop should the record be longer
than it.

> I encountered this on 9.3, but the patch applies to both 9.3 and master. I
> haven't tested it on other branches, but I have no reason to believe it
> won't apply or work. I believe we should back patch it all supported
> branches.

Agreed, that's a bug, and the logic of your patch looks good to me.

+   /*
+* Compute targetRecOff. It should typically be greater than short
+* page-header since a valid record can't , but can also be zero when
+* caller has supplied a page-aligned address or when we are skipping
+* multi-page continuation record. It doesn't matter though because
+* ReadPageInternal() will read at least short page-header worth of
+* data
+*/
This needs some polishing, there is an unfinished sentence here.

+   targetRecOff = tmpRecPtr % XLOG_BLCKSZ;
targetRecOff, pageHeaderSize and targetPagePtr could be declared
inside directly the new while loop.
-- 
Michael


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

Re: [HACKERS] pg_xlogdump and .partial files

[Michael Paquier]

On Mon, Sep 28, 2015 at 9:47 AM, Peter Eisentraut  wrote:
> The pg_xlogdump man page states that it cannot read .partial WAL files
> and that they need to be renamed.  It seems to me with about the same
> number of lines we could just make it accept those files, no?

FWIW, the discussion has happened here:
http://www.postgresql.org/message-id/cab7npqs_x5-m+s9yo_xd7oxwq1etvwv-1tsjuqnovqyyazt...@mail.gmail.com
In short, I was under the impression that this was not worth it, and I
still am under this impression knowing that this is not a common use
case.

Now, and I imagine that this is actually what is disturbing for the
user: we could allow the definition of a .partial file as an end
segment. We would need to be careful though in XLogDumpXLogRead when
doing the segment switch when the last segment is being read. As
pg_xlogdump does not handle timeline jumps this would be useful to
allow the user to have a look at the segments of a previous timeline
up to the point the promoted standby has switched to, particularly if
the .partial file, archived by a promoted standby, and its complete
version, archived by old master are present at the same position.
Still this seems like a very narrow use-case to me, so opinions are
welcome.
Regards,
-- 
Michael


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

[HACKERS] pg_xlogdump and .partial files

[Peter Eisentraut]

The pg_xlogdump man page states that it cannot read .partial WAL files
and that they need to be renamed.  It seems to me with about the same
number of lines we could just make it accept those files, no?


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

Re: [HACKERS] pg_xlogdump MSVC build script oddities

[Michael Paquier]

On Thu, Mar 19, 2015 at 11:25 AM, Heikki Linnakangas  wrote:
> I'm guessing that the current state of affairs is just an oversight. I tried
> changing it so that xlogreader.c is built into pg_xlogdump without copying,
> and it seemed to work. But I'm using a very recent version of MSVC - perhaps
> it won't work on pre-VS2011 versions.
>
> Unless someone has some insights on this, I'm going to commit the attached,
> and see what the buildfarm thinks of it.

This looks good to me. I just tested your patch on MS 2010, and it
worked, but that's perhaps not old enough to trigger the problem. Now
currawong and mastodon would be the real tests..
Regards,
-- 
Michael


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

[HACKERS] pg_xlogdump MSVC build script oddities

[Heikki Linnakangas]

When a frontend program needs to compile a file from another directory, 
Mkvcbuild.pm usually does something like this:



$pgdumpall->AddFile('src\bin\pg_dump\keywords.c');
$pgdumpall->AddFile('src\backend\parser\kwlookup.c');


But for pg_xlogdump, it does this:


foreach my $xf (glob('src/backend/access/rmgrdesc/*desc.c'))
{
my $bf = basename $xf;
copy($xf, "contrib/pg_xlogdump/$bf");
$pg_xlogdump->AddFile("contrib\\pg_xlogdump\\$bf");
}
copy(
'src/backend/access/transam/xlogreader.c',
'contrib/pg_xlogdump/xlogreader.c');


I.e. usually we instruct MSBuild to compile the source file from where 
it is, but for pg_xlogdump, we copy the source file. Is there a reason 
for this?


This was done by this commit:


commit a64e33f030f3ba379a0d3e22fe6bcda9dc3bbc60
Author: Andrew Dunstan 
Date:   Mon Feb 25 12:00:53 2013 -0500

Redo MSVC build implementation for pg_xlogdump.

The previous commit didn't work on MSVC editions earlier than
Visual Studio 2011, apparently. This works by copying files into the
contrib directory, and making provision to clean them up, which should
work on all editions.


Which followed this one:


commit 786170d74f30bc8d3017149dc444f3f3e29029a7
Author: Andrew Dunstan 
Date:   Sun Feb 24 20:28:42 2013 -0500

Provide MSVC build setup for pg_xlogdump.


But that earlier commit didn't use the straight AddFile approach either.

I'm guessing that the current state of affairs is just an oversight. I 
tried changing it so that xlogreader.c is built into pg_xlogdump without 
copying, and it seemed to work. But I'm using a very recent version of 
MSVC - perhaps it won't work on pre-VS2011 versions.


Unless someone has some insights on this, I'm going to commit the 
attached, and see what the buildfarm thinks of it.


- Heikki
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 989a2ec..473a310 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -13,7 +13,6 @@ use Project;
 use Solution;
 use Cwd;
 use File::Copy;
-use File::Basename;
 use Config;
 use VSObjectFactory;
 use List::Util qw(first);
@@ -628,15 +627,11 @@ sub mkvcbuild
 	  (grep { $_->{name} eq 'pg_xlogdump' }
 		  @{ $solution->{projects}->{contrib} })[0];
 	$pg_xlogdump->AddDefine('FRONTEND');
-	foreach my $xf (glob('src/backend/access/rmgrdesc/*desc.c'))
+	foreach my $xf (glob('src\\backend\\access\\rmgrdesc\\*desc.c'))
 	{
-		my $bf = basename $xf;
-		copy($xf, "contrib/pg_xlogdump/$bf");
-		$pg_xlogdump->AddFile("contrib\\pg_xlogdump\\$bf");
+		$pg_xlogdump->AddFile($xf)
 	}
-	copy(
-		'src/backend/access/transam/xlogreader.c',
-		'contrib/pg_xlogdump/xlogreader.c');
+	$pg_xlogdump->AddFile('src\backend\access\transam\xlogreader.c');
 
 	$solution->Save();
 	return $solution->{vcver};
diff --git a/src/tools/msvc/clean.bat b/src/tools/msvc/clean.bat
index 9c7ea42..fbe3cc6 100755
--- a/src/tools/msvc/clean.bat
+++ b/src/tools/msvc/clean.bat
@@ -92,11 +92,6 @@ REM Clean up datafiles built with contrib
 REM cd contrib
 REM for /r %%f in (*.sql) do if exist %%f.in del %%f
 
-REM clean up files copied into contrib\pg_xlogdump
-if exist contrib\pg_xlogdump\xlogreader.c del /q contrib\pg_xlogdump\xlogreader.c
-for %%f in (contrib\pg_xlogdump\*desc.c) do if not %%f==contrib\pg_xlogdump\rmgrdesc.c del /q %%f
-
-
 cd %D%
 
 REM Clean up ecpg regression test files

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

Re: [HACKERS] pg_xlogdump --stats

[Andres Freund]

On 2014-09-19 19:34:08 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > Hi,
> > 
> > On 2014-09-19 15:39:37 +0530, Abhijit Menon-Sen wrote:
> > > I've attached the revised patch, split up differently:
> > > 
> > > 1. Introduce rm_identify, change rm_desc, glue the two together in
> > >xlog.c
> > > 2. Introduce pg_xlogdump --stats[=record].
> > 
> > I've pushed these after some fixing up.
> 
> Hm, did you keep the static thingy you mentioned upthread (append_init
> which is duped unless I read wrong)?

Yes, I did.

>  There's quite some angst due to pg_dump's fmtId() -- I don't think we
> should be introducing more such things ...

I don't think these two are comparable. I don't really see many more
users of this springing up and printing the identity of two different
records in the same printf doesn't seem likely either.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump --stats

[Alvaro Herrera]

Andres Freund wrote:
> Hi,
> 
> On 2014-09-19 15:39:37 +0530, Abhijit Menon-Sen wrote:
> > I've attached the revised patch, split up differently:
> > 
> > 1. Introduce rm_identify, change rm_desc, glue the two together in
> >xlog.c
> > 2. Introduce pg_xlogdump --stats[=record].
> 
> I've pushed these after some fixing up.

Hm, did you keep the static thingy you mentioned upthread (append_init
which is duped unless I read wrong)?  There's quite some angst due to
pg_dump's fmtId() -- I don't think we should be introducing more such
things ...

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump --stats

[Andres Freund]

Hi,

On 2014-09-19 15:39:37 +0530, Abhijit Menon-Sen wrote:
> I've attached the revised patch, split up differently:
> 
> 1. Introduce rm_identify, change rm_desc, glue the two together in
>xlog.c
> 2. Introduce pg_xlogdump --stats[=record].

I've pushed these after some fixing up.

As we previously discussed, I think we might want to adjust the stats
further. But this is already really useful.

Thanks!

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-09-19 15:39:37 +0530, a...@2ndquadrant.com wrote:
>
> I hope I didn't miss anything this time.

But of course I did. The attached fixup makes the output of pg_xlogdump
match that of xlog_outdesc for unidentifiable records (UNKNOWN (%x)).
Sorry for the inconvenience.

-- Abhijit
diff --git a/contrib/pg_xlogdump/pg_xlogdump.c b/contrib/pg_xlogdump/pg_xlogdump.c
index 0a176bb..7686a4f 100644
--- a/contrib/pg_xlogdump/pg_xlogdump.c
+++ b/contrib/pg_xlogdump/pg_xlogdump.c
@@ -515,7 +515,7 @@ XLogDumpDisplayStats(XLogDumpConfig *config, XLogDumpStats *stats)
 
 id = desc->rm_identify(rj << 4);
 if (id == NULL)
-	id = psprintf("0x%x", rj << 4);
+	id = psprintf("UNKNOWN (%x)", rj << 4);
 
 XLogDumpStatsRow(psprintf("%s/%s", desc->rm_name, id),
  count, 100 * (double)count / total_count,

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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

I've attached the revised patch, split up differently:

1. Introduce rm_identify, change rm_desc, glue the two together in
   xlog.c
2. Introduce pg_xlogdump --stats[=record].

The requested changes (16, filter, z:, UNKNOWN) are included. The
grammar nitpicking and rebase cruft is^Ware^Wain't included.

I ran Postgres with WAL_DEBUG for a while, and I ran pg_xlogdump with
--stats (and --stats=rmgr) and --stats=record with and without a few
different variants of "-r heap". Everything looked OK to me.

I hope I didn't miss anything this time.

-- Abhijit
>From da6df2f24bb8ea768d6e7671474b0ad7d0b798d1 Mon Sep 17 00:00:00 2001
From: Abhijit Menon-Sen 
Date: Fri, 19 Sep 2014 15:08:45 +0530
Subject: Introduce an RmgrDescData.rm_identify callback to name records
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

For example, rm_identify turns XLOG_BTREE_VACUUM into "VACUUM" based on
xl_info. rm_desc now omits the prefix that (unsystematically) duplicated
the rm_identity output, and only provides extra detail if available:

LOG:  INSERT @ 0/16C50F0: prev 0/16C5080; xid 690; len 31: Heap/INSERT: rel 1663/16384/16385; tid 0/3

In the above, "Heap" comes from rm_name, "INSERT" comes from
rm_identify, and "rel 1663/…" comes from rm_desc.

The three are glued together by a new xlog_outdesc function in xlog.c.
---
 contrib/pg_xlogdump/rmgrdesc.c|   2 +-
 src/backend/access/rmgrdesc/clogdesc.c|  27 ---
 src/backend/access/rmgrdesc/dbasedesc.c   |  24 +-
 src/backend/access/rmgrdesc/gindesc.c |  54 ++---
 src/backend/access/rmgrdesc/gistdesc.c|  25 +-
 src/backend/access/rmgrdesc/hashdesc.c|   6 ++
 src/backend/access/rmgrdesc/heapdesc.c| 123 +
 src/backend/access/rmgrdesc/mxactdesc.c   |  37 ++---
 src/backend/access/rmgrdesc/nbtdesc.c | 124 +++---
 src/backend/access/rmgrdesc/relmapdesc.c  |  19 -
 src/backend/access/rmgrdesc/seqdesc.c |  21 +++--
 src/backend/access/rmgrdesc/smgrdesc.c|  25 --
 src/backend/access/rmgrdesc/spgdesc.c |  59 +++---
 src/backend/access/rmgrdesc/standbydesc.c |  25 --
 src/backend/access/rmgrdesc/tblspcdesc.c  |  25 --
 src/backend/access/rmgrdesc/xactdesc.c|  48 +---
 src/backend/access/rmgrdesc/xlogdesc.c|  72 -
 src/backend/access/transam/rmgr.c |   4 +-
 src/backend/access/transam/xlog.c |  45 +--
 src/include/access/clog.h |   1 +
 src/include/access/gin.h  |   1 +
 src/include/access/gist_private.h |   1 +
 src/include/access/hash.h |   1 +
 src/include/access/heapam_xlog.h  |   2 +
 src/include/access/multixact.h|   1 +
 src/include/access/nbtree.h   |   1 +
 src/include/access/rmgr.h |   2 +-
 src/include/access/rmgrlist.h |  34 
 src/include/access/spgist.h   |   1 +
 src/include/access/xact.h |   1 +
 src/include/access/xlog.h |   1 +
 src/include/access/xlog_internal.h|  11 +++
 src/include/catalog/storage_xlog.h|   1 +
 src/include/commands/dbcommands.h |   1 +
 src/include/commands/sequence.h   |   1 +
 src/include/commands/tablespace.h |   1 +
 src/include/storage/standby.h |   1 +
 src/include/utils/relmapper.h |   1 +
 38 files changed, 597 insertions(+), 232 deletions(-)

diff --git a/contrib/pg_xlogdump/rmgrdesc.c b/contrib/pg_xlogdump/rmgrdesc.c
index cbcaaa6..1064d34 100644
--- a/contrib/pg_xlogdump/rmgrdesc.c
+++ b/contrib/pg_xlogdump/rmgrdesc.c
@@ -27,7 +27,7 @@
 #include "storage/standby.h"
 #include "utils/relmapper.h"
 
-#define PG_RMGR(symname,name,redo,desc,startup,cleanup) \
+#define PG_RMGR(symname,name,redo,desc,identify,startup,cleanup) \
 	{ name, desc, },
 
 const RmgrDescData RmgrDescTable[RM_MAX_ID + 1] = {
diff --git a/src/backend/access/rmgrdesc/clogdesc.c b/src/backend/access/rmgrdesc/clogdesc.c
index e82baa8..8beb6d0 100644
--- a/src/backend/access/rmgrdesc/clogdesc.c
+++ b/src/backend/access/rmgrdesc/clogdesc.c
@@ -23,20 +23,29 @@ clog_desc(StringInfo buf, XLogRecord *record)
 	char	   *rec = XLogRecGetData(record);
 	uint8		info = record->xl_info & ~XLR_INFO_MASK;
 
-	if (info == CLOG_ZEROPAGE)
+	if (info == CLOG_ZEROPAGE || info == CLOG_TRUNCATE)
 	{
 		int			pageno;
 
 		memcpy(&pageno, rec, sizeof(int));
-		appendStringInfo(buf, "zeropage: %d", pageno);
+		appendStringInfo(buf, "%d", pageno);
 	}
-	else if (info == CLOG_TRUNCATE)
-	{
-		int			pageno;
+}
 
-		memcpy(&pageno, rec, sizeof(int));
-		appendStringInfo(buf, "truncate before: %d", pageno);
+const char *
+clog_identify(uint8 info)
+{
+	const char *id = NULL;
+
+	switch (info)
+	{
+		case CLOG_ZEROPAGE:
+			id = "ZEROPAGE";
+			break;
+		case CLOG_TRUNCATE:
+			id = "TRUNCATE";
+			break;
 	}
-	else
-		appendStringInfoString

Re: [HACKERS] pg_xlogdump --stats

[Andres Freund]

Hi,

On 2014-09-19 14:38:29 +0530, Abhijit Menon-Sen wrote:
> > b) I'm not against it, but I wonder if the best way to add the
> >SizeOfXLogRecord to the record size. It's just as much part of the
> >FPI. And this means that the record length will be > 0 even if all
> >the record data has been removed due to the FPI.
> 
> I'm not sure I understand what you are proposing here.

I'm not really proposing anything. I'm just stating that it's
notationally not clear and might be improved. Doesn't need to be now.

> > What was the reason you moved away from --stats=record/rmgr? I think
> > we possibly will add further ones, so that seems more extensible?
> 
> It was because I wanted --stats to default to "=rmgr", so I tried to
> make the argument optional, but getopt in Windows didn't like that.
> Here's an excerpt from the earlier discussion:
> 
> > 3. Some compilation error in windows
> > .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2065: 
> 'optional_argument' : undeclared identifier
> > .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2099: initializer is 
> not a constant
> >
> > optional_argument should be added to getopt_long.h file for windows.
> 
> Hmm. I have no idea what to do about this. I did notice when I wrote the
> code that nothing else used optional_argument, but I didn't realise that
> it wouldn't work on Windows.
> 
> It may be that the best thing to do would be to avoid using
> optional_argument altogether, and have separate --stats and
> --stats-per-record options. Thoughts?

Oh, I've since implemented optional arguments for windows/replacement
getopt_long. It's used by psql since a week or so ago.

> I have no objection to doing it differently if someone tells me how to
> make Windows happy (preferably without making me unhappy).

[x] Done

> > Given that you've removed the UNKNOWNs from the rm_descs, this really
> > should add it here.
> 
> You would prefer to see HEAP/UNKNOWN rather than HEAP/32 for an
> unidentifiable xl_info?

UNKNOWN (32)? It should visually stand out more than just the
number. Somebody borked something if that happens.

Greetings,

Andres Freund


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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-09-19 10:44:48 +0200, and...@2ndquadrant.com wrote:
>
> >  # snprintfs that use %lld, %qd, or %I64d as the format.  If none of these
> > -# work, fall back to our own snprintf emulation (which we know uses %lld).
> > +# works, fall back to our own snprintf emulation (which we know uses %lld).
> 
> spurious independent change?

It was part of the original patch, but I guess Heikki didn't commit it,
so it was left over in the rebase.

> Also I actually think the original version is correct?

It is not. I suspect it will begin to sound wrong to you if you replace
"none" with "not one" in the sentence. But I don't care enough to argue
about it. It's a very common error.

> > +   Stats   record_stats[RM_NEXT_ID][16];
> > +} XLogDumpStats;
> 
> I dislike the literal 16 here and someplace later. A define for the max
> number of records would make it clearer.

OK, will change.

> Perhaps we should move these kind of checks outside?

OK, will change.

> a) Whoever introduced the notion of rec_len vs tot_len in regards to
>including/excluding SizeOfXLogRecord ...

(It wasn't me, honest!)

> b) I'm not against it, but I wonder if the best way to add the
>SizeOfXLogRecord to the record size. It's just as much part of the
>FPI. And this means that the record length will be > 0 even if all
>the record data has been removed due to the FPI.

I'm not sure I understand what you are proposing here.

> What was the reason you moved away from --stats=record/rmgr? I think
> we possibly will add further ones, so that seems more extensible?

It was because I wanted --stats to default to "=rmgr", so I tried to
make the argument optional, but getopt in Windows didn't like that.
Here's an excerpt from the earlier discussion:

> 3. Some compilation error in windows
> .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2065: 
'optional_argument' : undeclared identifier
> .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2099: initializer is 
not a constant
>
> optional_argument should be added to getopt_long.h file for windows.

Hmm. I have no idea what to do about this. I did notice when I wrote the
code that nothing else used optional_argument, but I didn't realise that
it wouldn't work on Windows.

It may be that the best thing to do would be to avoid using
optional_argument altogether, and have separate --stats and
--stats-per-record options. Thoughts?

I have no objection to doing it differently if someone tells me how to
make Windows happy (preferably without making me unhappy).

> It's trivial to separate in this case, but I'd much rather have
> patches like this rm_identity stuff split up in the future.

Sorry. I'd split it up that way in the past, but forgot to do it again
in this round. Will do when I resend with the changes above.

> That means the returned value from heap_identity() is only valid until
> the next call. That at the very least needs to be written down
> explicitly somewhere.

Where? In the comment in xlog_internal.h, perhaps?

> That's already in there afaics:

Will fix (rebase cruft).

> Given that you've removed the UNKNOWNs from the rm_descs, this really
> should add it here.

You would prefer to see HEAP/UNKNOWN rather than HEAP/32 for an
unidentifiable xl_info?

-- Abhijit


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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-09-19 13:24:11 +0530, a...@2ndquadrant.com wrote:
>
> Good enough?

Not quite. I've attached a small additional patch that shifts the
responsibility of adding rm_name to the output from xlog_outrec to
xlog_outdesc. Now we get WAL_DEBUG output like this:

LOG:  INSERT @ 0/16C51D0: prev 0/16C5160; xid 692; len 31: Heap/INSERT: rel 
1663/16384/16385; tid 0/5

…which is consistent with pg_xlogdump --stats output too.

-- Abhijit
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1265eca..b9bf206 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1287,7 +1287,7 @@ begin:;
 			for (; rdata != NULL; rdata = rdata->next)
 appendBinaryStringInfo(&recordbuf, rdata->data, rdata->len);
 
-			appendStringInfoString(&buf, " - ");
+			appendStringInfoString(&buf, ": ");
 			xlog_outdesc(&buf, rechdr->xl_rmid, (XLogRecord *) recordbuf.data);
 		}
 		elog(LOG, "%s", buf.data);
@@ -6710,7 +6710,7 @@ StartupXLOG(void)
 			(uint32) (ReadRecPtr >> 32), (uint32) ReadRecPtr,
 			 (uint32) (EndRecPtr >> 32), (uint32) EndRecPtr);
 	xlog_outrec(&buf, record);
-	appendStringInfoString(&buf, " - ");
+	appendStringInfoString(&buf, ": ");
 	xlog_outdesc(&buf, record->xl_rmid, record);
 	elog(LOG, "%s", buf.data);
 	pfree(buf.data);
@@ -9625,8 +9625,6 @@ xlog_outrec(StringInfo buf, XLogRecord *record)
 		if (record->xl_info & XLR_BKP_BLOCK(i))
 			appendStringInfo(buf, "; bkpb%d", i);
 	}
-
-	appendStringInfo(buf, ": %s", RmgrTable[record->xl_rmid].rm_name);
 }
 #endif   /* WAL_DEBUG */
 
@@ -9639,6 +9637,9 @@ xlog_outdesc(StringInfo buf, RmgrId rmid, XLogRecord *record)
 {
 	const char *id;
 
+	appendStringInfoString(buf, RmgrTable[rmid].rm_name);
+	appendStringInfoChar(buf, '/');
+
 	id = RmgrTable[rmid].rm_identify(record->xl_info);
 	if (id == NULL)
 		appendStringInfo(buf, "%X", record->xl_info);

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

Re: [HACKERS] pg_xlogdump --stats

[Andres Freund]

On 2014-09-19 13:24:11 +0530, Abhijit Menon-Sen wrote:
> diff --git a/configure.in b/configure.in
> index 1392277..c3c458c 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -1637,7 +1637,7 @@ fi
>  # If we found "long int" is 64 bits, assume snprintf handles it.  If
>  # we found we need to use "long long int", better check.  We cope with
>  # snprintfs that use %lld, %qd, or %I64d as the format.  If none of these
> -# work, fall back to our own snprintf emulation (which we know uses %lld).
> +# works, fall back to our own snprintf emulation (which we know uses %lld).

spurious independent change? Also I actually think the original version
is correct?


> +typedef struct XLogDumpStats
> +{
> + uint64  count;
> + Stats   rmgr_stats[RM_NEXT_ID];
> + Stats   record_stats[RM_NEXT_ID][16];
> +} XLogDumpStats;

I dislike the literal 16 here and someplace later. A define for the max
number of records would make it clearer.

>  /*
> + * Store per-rmgr and per-record statistics for a given record.
> + */
> +static void
> +XLogDumpCountRecord(XLogDumpConfig *config, XLogDumpStats *stats, XLogRecPtr 
> ReadRecPtr, XLogRecord *record)
> +{
> + RmgrId  rmid;
> + uint8   recid;
> +
> + if (config->filter_by_rmgr != -1 &&
> + config->filter_by_rmgr != record->xl_rmid)
> + return;
> +
> + if (config->filter_by_xid_enabled &&
> + config->filter_by_xid != record->xl_xid)
> + return;

Perhaps we should move these kind of checks outside? So
XLogDumpDisplayRecord and this don't have to repeat them. I sure hope
we'll get some more. I e.g. really, really would like to have a
relfilenode check once Heikki's changes to make that possible are in.

> + stats->count++;
> +
> + /* Update per-rmgr statistics */
> +
> + rmid = record->xl_rmid;
> +
> + stats->rmgr_stats[rmid].count++;
> + stats->rmgr_stats[rmid].rec_len +=
> + record->xl_len + SizeOfXLogRecord;
> + stats->rmgr_stats[rmid].fpi_len +=
> + record->xl_tot_len - (record->xl_len + SizeOfXLogRecord);

a) Whoever introduced the notion of rec_len vs tot_len in regards to
   including/excluding SizeOfXLogRecord ...

b) I'm not against it, but I wonder if the best way to add the
   SizeOfXLogRecord to the record size. It's just as much part of the
   FPI. And this means that the record length will be > 0 even if all
   the record data has been removed due to the FPI.

>  static void
>  usage(void)
>  {
> @@ -401,6 +581,8 @@ usage(void)
>   printf(" (default: 1 or the value used in 
> STARTSEG)\n");
>   printf("  -V, --version  output version information, then 
> exit\n");
>   printf("  -x, --xid=XID  only show records with TransactionId 
> XID\n");
> + printf("  -z, --statsshow per-rmgr statistics\n");
> + printf("  -Z, --record-stats show per-record statistics\n");
>   printf("  -?, --help show this help, then exit\n");
>  }

What was the reason you moved away from --stats=record/rmgr? I think we
possibly will add further ones, so that seems more
extensible?

> diff --git a/contrib/pg_xlogdump/rmgrdesc.c b/contrib/pg_xlogdump/rmgrdesc.c
> index cbcaaa6..dc27fd1 100644
> --- a/contrib/pg_xlogdump/rmgrdesc.c
> +++ b/contrib/pg_xlogdump/rmgrdesc.c

It's trivial to separate in this case, but I'd much rather have patches
like this rm_identity stuff split up in the future.


> diff --git a/src/backend/access/rmgrdesc/heapdesc.c 
> b/src/backend/access/rmgrdesc/heapdesc.c
> index 24b6f92..91a8e72 100644
> --- a/src/backend/access/rmgrdesc/heapdesc.c
> +++ b/src/backend/access/rmgrdesc/heapdesc.c
> @@ -193,3 +193,86 @@ heap2_desc(StringInfo buf, XLogRecord *record)
>   else
>   appendStringInfoString(buf, "UNKNOWN");
>  }
> +
> +static const char *
> +append_init(const char *str)
> +{
> + static char x[32];
> +
> + strcpy(x, str);
> + strcat(x, "+INIT");
> +
> + return x;
> +}
> +
> +const char *
> +heap_identify(uint8 info)
> +{
> + const char *id = NULL;
> +
> + switch (info & XLOG_HEAP_OPMASK)
> + {
> + case XLOG_HEAP_INSERT:
> + id = "INSERT";
> + break;
> + case XLOG_HEAP_DELETE:
> + id = "DELETE";
> + break;
> + case XLOG_HEAP_UPDATE:
> + id = "UPDATE";
> + break;
> + case XLOG_HEAP_HOT_UPDATE:
> + id = "HOT_UPDATE";
> + break;
> + case XLOG_HEAP_LOCK:
> + id = "LOCK";
> + break;
> + case XLOG_HEAP_INPLACE:
> + id = "INPLACE";
> + break;
> + }
> +
> + if (info & XLOG_HEAP_INIT_PAGE)
> + id = append_init(id);
> +
> + return id;
> +}

Hm. I'm a bit doubtful

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

Hi.

I've attached two patches here:

0001-Make-pg_xlogdump-record-stats-display-summary-statis.patch is my
earlier patch to pg_xlogdump, rebased to master. It introduces the new
rm_identify callback, but doesn't touch rm_desc. Other than rebasing to
master after the INT64_FORMAT changes, I haven't changed anything.

0002-Clearly-separate-rm_identify-and-rm_desc-outputs.patch then makes
the change you (Heikki) wanted to see: rm_identify returns a name, and
rm_desc can be used to obtain optional additional detail, and xlog.c
just glues the two together in a new xlog_outdesc function. rm_desc
is changed largely only to (a) remove the "prefix: ", and (b) not
append UNKNOWN for unidentified records.

(I've done a little cleaning-up in the second patch, e.g. nbtdesc.c had
a bunch of repeated cases that I've unified, which seems a pretty nice
readability improvement overall.)

Good enough?

-- Abhijit
>From a85a5906733ca1324f7fceb6c4ff5b968a70532e Mon Sep 17 00:00:00 2001
From: Abhijit Menon-Sen 
Date: Wed, 4 Jun 2014 14:22:33 +0530
Subject: Make 'pg_xlogdump --[record-]stats' display summary statistics

---
 configure |   2 +-
 configure.in  |   2 +-
 contrib/pg_xlogdump/pg_xlogdump.c | 207 +-
 contrib/pg_xlogdump/rmgrdesc.c|   4 +-
 contrib/pg_xlogdump/rmgrdesc.h|   1 +
 doc/src/sgml/pg_xlogdump.sgml |  22 
 src/backend/access/rmgrdesc/clogdesc.c|  18 +++
 src/backend/access/rmgrdesc/dbasedesc.c   |  18 +++
 src/backend/access/rmgrdesc/gindesc.c |  42 ++
 src/backend/access/rmgrdesc/gistdesc.c|  21 +++
 src/backend/access/rmgrdesc/hashdesc.c|   6 +
 src/backend/access/rmgrdesc/heapdesc.c|  83 
 src/backend/access/rmgrdesc/mxactdesc.c   |  21 +++
 src/backend/access/rmgrdesc/nbtdesc.c |  54 
 src/backend/access/rmgrdesc/relmapdesc.c  |  15 +++
 src/backend/access/rmgrdesc/seqdesc.c |  15 +++
 src/backend/access/rmgrdesc/smgrdesc.c|  18 +++
 src/backend/access/rmgrdesc/spgdesc.c |  39 ++
 src/backend/access/rmgrdesc/standbydesc.c |  18 +++
 src/backend/access/rmgrdesc/tblspcdesc.c  |  18 +++
 src/backend/access/rmgrdesc/xactdesc.c|  33 +
 src/backend/access/rmgrdesc/xlogdesc.c|  45 +++
 src/backend/access/transam/rmgr.c |   4 +-
 src/include/access/clog.h |   1 +
 src/include/access/gin.h  |   1 +
 src/include/access/gist_private.h |   1 +
 src/include/access/hash.h |   1 +
 src/include/access/heapam_xlog.h  |   2 +
 src/include/access/multixact.h|   1 +
 src/include/access/nbtree.h   |   1 +
 src/include/access/rmgr.h |   2 +-
 src/include/access/rmgrlist.h |  34 ++---
 src/include/access/spgist.h   |   1 +
 src/include/access/xact.h |   1 +
 src/include/access/xlog.h |   1 +
 src/include/access/xlog_internal.h|   1 +
 src/include/c.h   |   3 +
 src/include/catalog/storage_xlog.h|   1 +
 src/include/commands/dbcommands.h |   1 +
 src/include/commands/sequence.h   |   1 +
 src/include/commands/tablespace.h |   1 +
 src/include/storage/standby.h |   1 +
 src/include/utils/relmapper.h |   1 +
 43 files changed, 736 insertions(+), 27 deletions(-)

diff --git a/configure b/configure
index dac8e49..22eb857 100755
--- a/configure
+++ b/configure
@@ -13091,7 +13091,7 @@ fi
 # If we found "long int" is 64 bits, assume snprintf handles it.  If
 # we found we need to use "long long int", better check.  We cope with
 # snprintfs that use %lld, %qd, or %I64d as the format.  If none of these
-# work, fall back to our own snprintf emulation (which we know uses %lld).
+# works, fall back to our own snprintf emulation (which we know uses %lld).
 
 if test "$HAVE_LONG_LONG_INT_64" = yes ; then
   if test $pgac_need_repl_snprintf = no; then
diff --git a/configure.in b/configure.in
index 1392277..c3c458c 100644
--- a/configure.in
+++ b/configure.in
@@ -1637,7 +1637,7 @@ fi
 # If we found "long int" is 64 bits, assume snprintf handles it.  If
 # we found we need to use "long long int", better check.  We cope with
 # snprintfs that use %lld, %qd, or %I64d as the format.  If none of these
-# work, fall back to our own snprintf emulation (which we know uses %lld).
+# works, fall back to our own snprintf emulation (which we know uses %lld).
 
 if test "$HAVE_LONG_LONG_INT_64" = yes ; then
   if test $pgac_need_repl_snprintf = no; then
diff --git a/contrib/pg_xlogdump/pg_xlogdump.c b/contrib/pg_xlogdump/pg_xlogdump.c
index c555786..11da5a8 100644
--- a/contrib/pg_xlogdump/pg_xlogdump.c
+++ b/contrib/pg_xlogdump/pg_xlogdump.c
@@ -15,9 +15,10 @@
 #include 
 #include 
 
-#include "access/xlog.h"
 #include "access/xlogreader.h"
 #include "access/transam.h"
+#include "catalog/pg

Re: [HACKERS] pg_xlogdump --stats

[Heikki Linnakangas]

On 09/11/2014 12:22 PM, Andres Freund wrote:

On 2014-09-11 12:14:42 +0300, Heikki Linnakangas wrote:

On 09/11/2014 11:43 AM, Abhijit Menon-Sen wrote:

At 2014-08-21 10:06:39 +0300, hlinnakan...@vmware.com wrote:

I think the names that rm_identify returns should match those that the
rm_desc functions print.


I had originally started off trying to keep the output in sync, but it
doesn't work very well. There are rm_desc functions that print things
like "truncate before" and "Create posting tree", and many decisions
are quite arbitrary ("freeze_page", "cleanup info", "multi-insert").


It would be good to clean up those messages to be more sensible and
consistent.



I think it's better the (grep-friendly) way it is. If anything, perhaps
rm_desc should output "${rm_identify}[: optional explanation]". That
would also make it very clear what should go in rm_identify and what
should go in rm_desc.



Yeah, that makes sense too.


I'm not sure I agree here. From a theoretical POV sure, but wouldn't
that lead to even longer lines for xlogdump and other error messages for
some records?


No. All the rm_desc functions already follow that convention, and print 
"foo: details", where "foo" identifies the record type based on xl_info. 
This proposal would just make that convention more official, and 
stipulate that the "foo" part should match what the new rm_identify() 
function returns. (Or perhaps even better, define it so that rm_desc 
prints only the "details" part, and rm_identify() the "foo" part, and 
have the callers put the two strings together if they want)



There are similar cases in B-tree splits, for
example, where a split where the new tuple goes to the left half is
considered a different record type than one where it goes ot the right half.
It might be interesting to count them separately in the stats, but then
again it might just be confusing. The xl_info flags and the rm_desc output
were never intended to be a useful categorization for statistics purposes,
so it's not surprising that it's not too well suited for that. Nevertheless,
the "pg_xlogdump --stats" is useful as it is, if you know the limitations.


I agree it's not perfect, but I think it's a huge step forward. We very
well might want to improve upon the stats granularity once in, but I
think it already gives a fair amount of direction where to look.


Agreed. That's what I was also trying to say.

- Heikki



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

Re: [HACKERS] pg_xlogdump --stats

[Andres Freund]

On 2014-09-11 12:14:42 +0300, Heikki Linnakangas wrote:
> On 09/11/2014 11:43 AM, Abhijit Menon-Sen wrote:
> >At 2014-08-21 10:06:39 +0300, hlinnakan...@vmware.com wrote:
> >>I think the names that rm_identify returns should match those that the
> >>rm_desc functions print.
> >
> >I had originally started off trying to keep the output in sync, but it
> >doesn't work very well. There are rm_desc functions that print things
> >like "truncate before" and "Create posting tree", and many decisions
> >are quite arbitrary ("freeze_page", "cleanup info", "multi-insert").
> 
> It would be good to clean up those messages to be more sensible and
> consistent.

> >I think it's better the (grep-friendly) way it is. If anything, perhaps
> >rm_desc should output "${rm_identify}[: optional explanation]". That
> >would also make it very clear what should go in rm_identify and what
> >should go in rm_desc.

> Yeah, that makes sense too.

I'm not sure I agree here. From a theoretical POV sure, but wouldn't
that lead to even longer lines for xlogdump and other error messages for
some records?
We probably need to see how it plays out.

> >>The corresponding rm_identify output is:
> >>
> >>HOT_UPDATE+INIT
> >
> >The +INIT is admittedly a special case, and I would have no objection to
> >writing that as (INIT) or something instead.
> 
> I don't care much, as long as it's consistent the rm_desc output.
> 
> It's quite arbitrary that the INIT cases are considered different record
> types, with their own "identity" string, instead of being just a flag in the
> same record type. It's an implementation detail that that flag is stored in
> the xl_info, and that implementation detail is now exposed in the stats
> pg_xlogdump --stats output.

It's also actually quite good from a display purpose. +INIT will have
quite different wal logging characteristics :)

> There are similar cases in B-tree splits, for
> example, where a split where the new tuple goes to the left half is
> considered a different record type than one where it goes ot the right half.
> It might be interesting to count them separately in the stats, but then
> again it might just be confusing. The xl_info flags and the rm_desc output
> were never intended to be a useful categorization for statistics purposes,
> so it's not surprising that it's not too well suited for that. Nevertheless,
> the "pg_xlogdump --stats" is useful as it is, if you know the limitations.

I agree it's not perfect, but I think it's a huge step forward. We very
well might want to improve upon the stats granularity once in, but I
think it already gives a fair amount of direction where to look.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump --stats

[Heikki Linnakangas]

On 09/11/2014 11:43 AM, Abhijit Menon-Sen wrote:

At 2014-08-21 10:06:39 +0300, hlinnakan...@vmware.com wrote:

I think the names that rm_identify returns should match those that the
rm_desc functions print.


I had originally started off trying to keep the output in sync, but it
doesn't work very well. There are rm_desc functions that print things
like "truncate before" and "Create posting tree", and many decisions
are quite arbitrary ("freeze_page", "cleanup info", "multi-insert").


It would be good to clean up those messages to be more sensible and 
consistent.



I think it's better the (grep-friendly) way it is. If anything, perhaps
rm_desc should output "${rm_identify}[: optional explanation]". That
would also make it very clear what should go in rm_identify and what
should go in rm_desc.


Yeah, that makes sense too.


The corresponding rm_identify output is:

HOT_UPDATE+INIT


The +INIT is admittedly a special case, and I would have no objection to
writing that as (INIT) or something instead.


I don't care much, as long as it's consistent the rm_desc output.

It's quite arbitrary that the INIT cases are considered different record 
types, with their own "identity" string, instead of being just a flag in 
the same record type. It's an implementation detail that that flag is 
stored in the xl_info, and that implementation detail is now exposed in 
the stats pg_xlogdump --stats output. There are similar cases in B-tree 
splits, for example, where a split where the new tuple goes to the left 
half is considered a different record type than one where it goes ot the 
right half. It might be interesting to count them separately in the 
stats, but then again it might just be confusing. The xl_info flags and 
the rm_desc output were never intended to be a useful categorization for 
statistics purposes, so it's not surprising that it's not too well 
suited for that. Nevertheless, the "pg_xlogdump --stats" is useful as it 
is, if you know the limitations.


- Heikki



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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-08-21 10:06:39 +0300, hlinnakan...@vmware.com wrote:
>
> Committed the patch to add INT64_MODIFIER, with minor fixes.

Thank you.

> The new rm_identify method needs to be documented. […]
> Perhaps add comments to the RmgrData struct, explaining
> all of the methods.

OK, I'll submit a patch to add these comments.

> I think the names that rm_identify returns should match those that the
> rm_desc functions print.

I had originally started off trying to keep the output in sync, but it
doesn't work very well. There are rm_desc functions that print things
like "truncate before" and "Create posting tree", and many decisions
are quite arbitrary ("freeze_page", "cleanup info", "multi-insert").

I think it's better the (grep-friendly) way it is. If anything, perhaps
rm_desc should output "${rm_identify}[: optional explanation]". That
would also make it very clear what should go in rm_identify and what
should go in rm_desc.

Thoughts?

> The corresponding rm_identify output is:
> 
> HOT_UPDATE+INIT

The +INIT is admittedly a special case, and I would have no objection to
writing that as (INIT) or something instead.

-- Abhijit


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

Re: [HACKERS] pg_xlogdump --stats

[Heikki Linnakangas]

On 07/07/2014 10:32 PM, Abhijit Menon-Sen wrote:

At 2014-07-07 09:48:44 +0200, and...@2ndquadrant.com wrote:


I'd suggest only defining INT64_MODIFIER here and build INT64_FORMAT,
UINT64_FORMAT ontop, in c.h.


Oh, I see. I'm sorry, I misread your earlier suggestion. Regenerated
patches attached. Is this what you had in mind?


Committed the patch to add INT64_MODIFIER, with minor fixes.

The new rm_identify method needs to be documented. Not sure where; 
surprisingly I can't find any documentation on the existing methods 
either. Perhaps add comments to the RmgrData struct, explaining all of 
the methods. In particular, should give guidelines on what output 
belongs in rm_desc and what in rm_identify.


I think the names that rm_identify returns should match those that the 
rm_desc functions print. As the patch stands, for example when heap_desc 
prints out something like:


hot_update(init): xmax 123; new tid 1/2 xmax 456

The corresponding rm_identify output is:

HOT_UPDATE+INIT

It would be better to spell them the same, so that when you look at the 
summary stats and the raw pg_xlogdump output, you can tell which records 
contributed to which summary line.


- Heikki



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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-07-07 09:48:44 +0200, and...@2ndquadrant.com wrote:
>
> I'd suggest only defining INT64_MODIFIER here and build INT64_FORMAT,
> UINT64_FORMAT ontop, in c.h.

Oh, I see. I'm sorry, I misread your earlier suggestion. Regenerated
patches attached. Is this what you had in mind?

-- Abhijit
diff --git a/src/include/c.h b/src/include/c.h
index a48a57a..999f297 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -864,6 +864,9 @@ typedef NameData *Name;
  * 
  */
 
+#define INT64_FORMAT "%" INT64_MODIFIER "d"
+#define UINT64_FORMAT "%" INT64_MODIFIER "u"
+
 /* msb for char */
 #define HIGHBIT	(0x80)
 #define IS_HIGHBIT_SET(ch)		((unsigned char)(ch) & HIGHBIT)

diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 2a40d61..c2e01fd 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -650,8 +650,8 @@
 /* Define to 1 if your compiler understands __VA_ARGS__ in macros. */
 #undef HAVE__VA_ARGS
 
-/* Define to the appropriate snprintf format for 64-bit ints. */
-#undef INT64_FORMAT
+/* Define to the appropriate snprintf length modifier for 64-bit ints. */
+#undef INT64_MODIFIER
 
 /* Define to 1 if `locale_t' requires . */
 #undef LOCALE_T_IN_XLOCALE
@@ -741,9 +741,6 @@
 /* Define to 1 if your  declares `struct tm'. */
 #undef TM_IN_SYS_TIME
 
-/* Define to the appropriate snprintf format for unsigned 64-bit ints. */
-#undef UINT64_FORMAT
-
 /* Define to 1 to build with assertion checks. (--enable-cassert) */
 #undef USE_ASSERT_CHECKING
 
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index 1c9cd82..a238a72 100644
--- a/src/include/pg_config.h.win32
+++ b/src/include/pg_config.h.win32
@@ -529,8 +529,8 @@
 /* Define to 1 if your compiler understands __VA_ARGS__ in macros. */
 #define HAVE__VA_ARGS 1
 
-/* Define to the appropriate snprintf format for 64-bit ints, if any. */
-#define INT64_FORMAT "%lld"
+/* Define to the appropriate snprintf length modifier for 64-bit ints. */
+#undef INT64_MODIFIER
 
 /* Define to 1 if `locale_t' requires . */
 /* #undef LOCALE_T_IN_XLOCALE */
@@ -601,10 +601,6 @@
 /* Define to 1 if your  declares `struct tm'. */
 /* #undef TM_IN_SYS_TIME */
 
-/* Define to the appropriate snprintf format for unsigned 64-bit ints, if any.
-   */
-#define UINT64_FORMAT "%llu"
-
 /* Define to 1 to build with assertion checks. (--enable-cassert) */
 /* #undef USE_ASSERT_CHECKING */
 
diff --git a/configure.in b/configure.in
index c938a5d..7750c30 100644
--- a/configure.in
+++ b/configure.in
@@ -1636,34 +1636,29 @@ fi
 # If we found "long int" is 64 bits, assume snprintf handles it.  If
 # we found we need to use "long long int", better check.  We cope with
 # snprintfs that use %lld, %qd, or %I64d as the format.  If none of these
-# work, fall back to our own snprintf emulation (which we know uses %lld).
+# works, fall back to our own snprintf emulation (which we know uses %lld).
 
 if test "$HAVE_LONG_LONG_INT_64" = yes ; then
   if test $pgac_need_repl_snprintf = no; then
-PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT
-if test "$LONG_LONG_INT_FORMAT" = ""; then
+PGAC_FUNC_SNPRINTF_LONG_LONG_INT_MODIFIER
+if test "$LONG_LONG_INT_MODIFIER" = ""; then
   # Force usage of our own snprintf, since system snprintf is broken
   pgac_need_repl_snprintf=yes
-  LONG_LONG_INT_FORMAT='%lld'
+  LONG_LONG_INT_MODIFIER='ll'
 fi
   else
 # Here if we previously decided we needed to use our own snprintf
-LONG_LONG_INT_FORMAT='%lld'
+LONG_LONG_INT_MODIFIER='ll'
   fi
-  LONG_LONG_UINT_FORMAT=`echo "$LONG_LONG_INT_FORMAT" | sed 's/d$/u/'`
-  INT64_FORMAT="\"$LONG_LONG_INT_FORMAT\""
-  UINT64_FORMAT="\"$LONG_LONG_UINT_FORMAT\""
 else
   # Here if we are not using 'long long int' at all
-  INT64_FORMAT='"%ld"'
-  UINT64_FORMAT='"%lu"'
+  LONG_LONG_INT_MODIFIER='l'
 fi
 
-AC_DEFINE_UNQUOTED(INT64_FORMAT, $INT64_FORMAT,
-   [Define to the appropriate snprintf format for 64-bit ints.])
+INT64_MODIFIER="\"$LONG_LONG_INT_MODIFIER\""
 
-AC_DEFINE_UNQUOTED(UINT64_FORMAT, $UINT64_FORMAT,
-   [Define to the appropriate snprintf format for unsigned 64-bit ints.])
+AC_DEFINE_UNQUOTED(INT64_MODIFIER, $INT64_MODIFIER,
+   [Define to the appropriate snprintf length modifier for 64-bit ints.])
 
 # Also force use of our snprintf if the system's doesn't support the %z flag.
 if test "$pgac_need_repl_snprintf" = no; then

diff --git a/config/c-library.m4 b/config/c-library.m4
index 8f45010..4821a61 100644
--- a/config/c-library.m4
+++ b/config/c-library.m4
@@ -221,22 +221,22 @@ HAVE_POSIX_SIGNALS=$pgac_cv_func_posix_signals
 AC_SUBST(HAVE_POSIX_SIGNALS)])# PGAC_FUNC_POSIX_SIGNALS
 
 
-# PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT
+# PGAC_FUNC_SNPRINTF_LONG_LONG_INT_MODIFIER
 # ---
-# Determine which format snprintf uses for long long int.  We handle
-# %lld, %qd, %I64d.  The 

Re: [HACKERS] pg_xlogdump --stats

[Andres Freund]

On 2014-07-04 18:59:07 +0530, Abhijit Menon-Sen wrote:
> At 2014-07-04 11:34:21 +0200, and...@2ndquadrant.com wrote:
> >
> > I think we're going to have to redefine the
> > PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT callsite in configure.in to
> > define INT64_MODIFIER='"ll/l/I64D"'
> 
> I've attached a patch to do this, and also add INT64_MODIFIER to
> pg_config.h.in: 0001-modifier.diff
> 
> I reran autoconf, and just for convenience I've attached the resulting
> changes to configure: 0002-configure.diff
> 
> Then there are the rm_identify changes: 0003-rmid.diff
> 
> Finally, the xlogdump patch using INT64_MODIFIER: 0004-xlogdump.diff
> 
> I can confirm that this series applies in-order to master, and that the
> result builds cleanly (including after each patch) on my machine, and
> that the resulting pg_xlogdump works as expected.

> Two of the extra calls to psprintf in pg_xlogdump happen at most
> RM_MAX_ID*16 (i.e. O(record-types) not O(records)) times, and the other
> two happen just before exit. It would be easy to use a static buffer and
> snprintf, but I don't think it's worth doing in this case.

Agreed.

> diff --git a/config/c-library.m4 b/config/c-library.m4
> index 8f45010..4821a61 100644
> --- a/config/c-library.m4
> +++ b/config/c-library.m4
> @@ -221,22 +221,22 @@ HAVE_POSIX_SIGNALS=$pgac_cv_func_posix_signals
>  AC_SUBST(HAVE_POSIX_SIGNALS)])# PGAC_FUNC_POSIX_SIGNALS
>  
>  
> -# PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT
> +# PGAC_FUNC_SNPRINTF_LONG_LONG_INT_MODIFIER
>  # ---
> -# Determine which format snprintf uses for long long int.  We handle
> -# %lld, %qd, %I64d.  The result is in shell variable
> -# LONG_LONG_INT_FORMAT.
> +# Determine which length modifier snprintf uses for long long int.  We
> +# handle ll, q, and I64.  The result is in shell variable
> +# LONG_LONG_INT_MODIFIER.
>  #
>  # MinGW uses '%I64d', though gcc throws an warning with -Wall,
>  # while '%lld' doesn't generate a warning, but doesn't work.
>  #
> -AC_DEFUN([PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT],
> -[AC_MSG_CHECKING([snprintf format for long long int])
> -AC_CACHE_VAL(pgac_cv_snprintf_long_long_int_format,
> -[for pgac_format in '%lld' '%qd' '%I64d'; do
> +AC_DEFUN([PGAC_FUNC_SNPRINTF_LONG_LONG_INT_MODIFIER],
> +[AC_MSG_CHECKING([snprintf length modifier for long long int])
> +AC_CACHE_VAL(pgac_cv_snprintf_long_long_int_modifier,
> +[for pgac_modifier in 'll' 'q' 'I64'; do
>  AC_TRY_RUN([#include 
>  typedef long long int ac_int64;
> -#define INT64_FORMAT "$pgac_format"
> +#define INT64_FORMAT "%${pgac_modifier}d"

I'd rather not define INT64_FORMAT here.

> +INT64_FORMAT="\"%${LONG_LONG_INT_MODIFIER}d\""
> +UINT64_FORMAT="\"%${LONG_LONG_INT_MODIFIER}u\""
> +INT64_MODIFIER="\"$LONG_LONG_INT_MODIFIER\""
> +

>  AC_DEFINE_UNQUOTED(INT64_FORMAT, $INT64_FORMAT,
> [Define to the appropriate snprintf format for 64-bit 
> ints.])
>  
>  AC_DEFINE_UNQUOTED(UINT64_FORMAT, $UINT64_FORMAT,
> [Define to the appropriate snprintf format for unsigned 
> 64-bit ints.])
>  
> +AC_DEFINE_UNQUOTED(INT64_MODIFIER, $INT64_MODIFIER,
> +   [Define to the appropriate snprintf length modifier for 
> 64-bit ints.])
> +

I'd suggest only defining INT64_MODIFIER here and build INT64_FORMAT,
UINT64_FORMAT ontop, in c.h.

> NOTE: I do not know what to do about pg_config.h.win32. If someone tells
> me what to do, I can submit another patch.

Which would also take care of pg_config.h.win32 - just define
INT64_MODIFIER in there instead of INT64_FORMAT/UINT64_FORMAT and you
should be good.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-07-04 13:43:46 -0400, alvhe...@2ndquadrant.com wrote:
>
> Now, I see that pg_xlogdump is checking for NULL return, but I'm not
> sure this is the best possible API. 

Note that the patched pg_xlogdump will display "rm_name/0xNN" for any
records that rm_identify returns a NULL for.

Earlier, when there was the possibility that new record types could be
added without updating pg_xlogdump's identification code, this made good
sense. Now one could argue that pg_xlogdump should fully depend on every
record in WAL being properly identified.

If that's the case, rm_identify could return "UNKNOWN", and pg_xlogdump
could use the return value without checking. I considered that, but the
only thing that stopped me was the thought that if a "weird" record DOES
show up in WAL somehow, it'd be pretty handy to (a) know the value of
xl_info, and (b) see if there's more than one kind (per-rmid).

But I don't feel strongly enough about it that I'd object to displaying
just "UNKNOWN".

> I wonder if it'd be better to pass the whole record instead and have
> the rm_identify function pull out the info if it's all it needs, but
> leave the possibility open that it could read, say, some header bytes
> in the record data.

I don't *have* an XLogRecord at the point where I'm calling rm_identify.
I could call rm_identify earlier, and either store the name in the stats
structure, or hash the name and use the hash value to store that record
type's statistics.

We don't even have any other potential callers for rm_identify. Adding
it and making it significantly more difficult to use for the only code
that actually needs it… no, I pretty much hate that idea.

Personally, I think it's fine to keep rm_identify the way it is. It's
hardly very difficult code, and if the assumption that records can be
identified by xl_info ever becomes invalid, this isn't the only code
that will need to be changed either.

(Otherwise, I'd actually prefer to keep all the identification code in
pg_xlogdump, i.e. the pre-rm_identify version of my patch. At least that
would make it possible to maintain a version that could be built against
9.3, the way Marti did.)

> Also I didn't check how you handle the "init" bit in RM_HEAP and
> RM_HEAP2.  Are we just saying that "insert (init)" is a different
> record type from "insert"?

Yes, that's what the patch does currently. "X" vs. "X+INIT".

-- Abhijit


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

Re: [HACKERS] pg_xlogdump --stats

[Alvaro Herrera]

Andres Freund wrote:
> On 2014-07-04 18:31:34 +0800, gotoschool6g wrote:

> > Carp the code:
> > 
> > const char *
> > clog_identify(uint8 info)
> > { 
> >  switch (info)
> >  {
> >   case CLOG_ZEROPAGE:
> >return "ZEROPAGE";
> >   case CLOG_TRUNCATE:
> >return "TRUNCATE";
> >break;
> >  }
> >  return NULL;
> > }

I agree that performance is secondary here.  The part that I don't quite
like in all these blocks is the fact that they initialize the return
value to NULL beforehand, and there's no 'default' label.  Now, I see
that pg_xlogdump is checking for NULL return, but I'm not sure this is
the best possible API.  Shouldn't we perhaps return a different string
that indicates there is no known description?

Also, are we certain that we're never going to need anything other than
the "info" to identify the record?  In practice we seem to follow that
rule currently, but it's not totally out of the question that someday
the rm_redo function uses more than just "info" to identify the record.
I wonder if it'd be better to pass the whole record instead and have the
rm_identify function pull out the info if it's all it needs, but leave
the possibility open that it could read, say, some header bytes in the
record data.

Also I didn't check how you handle the "init" bit in RM_HEAP and
RM_HEAP2.  Are we just saying that "insert (init)" is a different record
type from "insert"?  Maybe that's okay, but I'm not 100% sure.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-07-04 11:34:21 +0200, and...@2ndquadrant.com wrote:
>
> I think we're going to have to redefine the
> PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT callsite in configure.in to
> define INT64_MODIFIER='"ll/l/I64D"'

I've attached a patch to do this, and also add INT64_MODIFIER to
pg_config.h.in: 0001-modifier.diff

I reran autoconf, and just for convenience I've attached the resulting
changes to configure: 0002-configure.diff

Then there are the rm_identify changes: 0003-rmid.diff

Finally, the xlogdump patch using INT64_MODIFIER: 0004-xlogdump.diff

I can confirm that this series applies in-order to master, and that the
result builds cleanly (including after each patch) on my machine, and
that the resulting pg_xlogdump works as expected.

NOTE: I do not know what to do about pg_config.h.win32. If someone tells
me what to do, I can submit another patch.

> Some additional leaking here.

Two of the extra calls to psprintf in pg_xlogdump happen at most
RM_MAX_ID*16 (i.e. O(record-types) not O(records)) times, and the other
two happen just before exit. It would be easy to use a static buffer and
snprintf, but I don't think it's worth doing in this case.

-- Abhijit, hoping with crossed fingers to not forget attachments now.
diff --git a/config/c-library.m4 b/config/c-library.m4
index 8f45010..4821a61 100644
--- a/config/c-library.m4
+++ b/config/c-library.m4
@@ -221,22 +221,22 @@ HAVE_POSIX_SIGNALS=$pgac_cv_func_posix_signals
 AC_SUBST(HAVE_POSIX_SIGNALS)])# PGAC_FUNC_POSIX_SIGNALS
 
 
-# PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT
+# PGAC_FUNC_SNPRINTF_LONG_LONG_INT_MODIFIER
 # ---
-# Determine which format snprintf uses for long long int.  We handle
-# %lld, %qd, %I64d.  The result is in shell variable
-# LONG_LONG_INT_FORMAT.
+# Determine which length modifier snprintf uses for long long int.  We
+# handle ll, q, and I64.  The result is in shell variable
+# LONG_LONG_INT_MODIFIER.
 #
 # MinGW uses '%I64d', though gcc throws an warning with -Wall,
 # while '%lld' doesn't generate a warning, but doesn't work.
 #
-AC_DEFUN([PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT],
-[AC_MSG_CHECKING([snprintf format for long long int])
-AC_CACHE_VAL(pgac_cv_snprintf_long_long_int_format,
-[for pgac_format in '%lld' '%qd' '%I64d'; do
+AC_DEFUN([PGAC_FUNC_SNPRINTF_LONG_LONG_INT_MODIFIER],
+[AC_MSG_CHECKING([snprintf length modifier for long long int])
+AC_CACHE_VAL(pgac_cv_snprintf_long_long_int_modifier,
+[for pgac_modifier in 'll' 'q' 'I64'; do
 AC_TRY_RUN([#include 
 typedef long long int ac_int64;
-#define INT64_FORMAT "$pgac_format"
+#define INT64_FORMAT "%${pgac_modifier}d"
 
 ac_int64 a = 2001;
 ac_int64 b = 4005;
@@ -258,19 +258,19 @@ int does_int64_snprintf_work()
 main() {
   exit(! does_int64_snprintf_work());
 }],
-[pgac_cv_snprintf_long_long_int_format=$pgac_format; break],
+[pgac_cv_snprintf_long_long_int_modifier=$pgac_modifier; break],
 [],
-[pgac_cv_snprintf_long_long_int_format=cross; break])
+[pgac_cv_snprintf_long_long_int_modifier=cross; break])
 done])dnl AC_CACHE_VAL
 
-LONG_LONG_INT_FORMAT=''
+LONG_LONG_INT_MODIFIER=''
 
-case $pgac_cv_snprintf_long_long_int_format in
+case $pgac_cv_snprintf_long_long_int_modifier in
   cross) AC_MSG_RESULT([cannot test (not on host machine)]);;
-  ?*)AC_MSG_RESULT([$pgac_cv_snprintf_long_long_int_format])
- LONG_LONG_INT_FORMAT=$pgac_cv_snprintf_long_long_int_format;;
+  ?*)AC_MSG_RESULT([$pgac_cv_snprintf_long_long_int_modifier])
+ LONG_LONG_INT_MODIFIER=$pgac_cv_snprintf_long_long_int_modifier;;
   *) AC_MSG_RESULT(none);;
-esac])# PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT
+esac])# PGAC_FUNC_SNPRINTF_LONG_LONG_INT_MODIFIER
 
 
 # PGAC_FUNC_SNPRINTF_ARG_CONTROL

diff --git a/configure.in b/configure.in
index c938a5d..6afc818 100644
--- a/configure.in
+++ b/configure.in
@@ -1636,35 +1636,38 @@ fi
 # If we found "long int" is 64 bits, assume snprintf handles it.  If
 # we found we need to use "long long int", better check.  We cope with
 # snprintfs that use %lld, %qd, or %I64d as the format.  If none of these
-# work, fall back to our own snprintf emulation (which we know uses %lld).
+# works, fall back to our own snprintf emulation (which we know uses %lld).
 
 if test "$HAVE_LONG_LONG_INT_64" = yes ; then
   if test $pgac_need_repl_snprintf = no; then
-PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT
-if test "$LONG_LONG_INT_FORMAT" = ""; then
+PGAC_FUNC_SNPRINTF_LONG_LONG_INT_MODIFIER
+if test "$LONG_LONG_INT_MODIFIER" = ""; then
   # Force usage of our own snprintf, since system snprintf is broken
   pgac_need_repl_snprintf=yes
-  LONG_LONG_INT_FORMAT='%lld'
+  LONG_LONG_INT_MODIFIER='ll'
 fi
   else
 # Here if we previously decided we needed to use our own snprintf
-LONG_LONG_INT_FORMAT='%lld'
+LONG_LONG_INT_MODIFIER='ll'
   fi
-  LONG_LONG_UINT_FORMAT=`echo "$LONG_LONG_INT_FORMAT" | sed 's/d$/u/'`
-  INT64_FORMAT="\"$LONG_LONG_INT_FORMAT\""
-  UINT64_FORMAT="\

Re: [HACKERS] pg_xlogdump --stats

[Andres Freund]

On 2014-07-04 18:31:34 +0800, gotoschool6g wrote:
> > 
> > I'm pretty sure that most committers would want to apply the 
> > rm_identity part in a separate commit first. Could you make it 
> > two patches, one introducing rm_identity, and another with 
> > xlogdump using it? 
> 
> Carp the code:
> 
> const char *
> clog_identify(uint8 info)
> { 
>  switch (info)
>  {
>   case CLOG_ZEROPAGE:
>return "ZEROPAGE";
>   case CLOG_TRUNCATE:
>return "TRUNCATE";
>break;
>  }
>  return NULL;
> }
> 
> or
> 
> const char *
> clog_identify(uint8 info)
> {
> if(info==CLOG_ZEROPAGE)return "ZEROPAGE";
> if(info==CLOG_TRUNCATE)return "TRUNCATE";
> return NULL;
> }
> 
> is a bit faster than:

Any halfway decent compiler will not use a local variable here. Don't
think that matters much. Also the code isn't a performance bottleneck...

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump --stats

[gotoschool6g]

> 
> I'm pretty sure that most committers would want to apply the 
> rm_identity part in a separate commit first. Could you make it 
> two patches, one introducing rm_identity, and another with 
> xlogdump using it? 

Carp the code:

const char *
clog_identify(uint8 info)
{ 
 switch (info)
 {
  case CLOG_ZEROPAGE:
   return "ZEROPAGE";
  case CLOG_TRUNCATE:
   return "TRUNCATE";
   break;
 }
 return NULL;
}

or

const char *
clog_identify(uint8 info)
{
if(info==CLOG_ZEROPAGE)return "ZEROPAGE";
if(info==CLOG_TRUNCATE)return "TRUNCATE";
return NULL;
}

is a bit faster than:

const char *
clog_identify(uint8 info)
{
 const char *id = NULL;

 switch (info)
 {
  case CLOG_ZEROPAGE:
   id = "ZEROPAGE";
   break;
  case CLOG_TRUNCATE:
   id = "TRUNCATE";
   break;
 }

 return id;
}

Re: [HACKERS] pg_xlogdump --stats

[Andres Freund]

On 2014-07-04 15:34:14 +0530, Abhijit Menon-Sen wrote:
> At 2014-07-04 11:34:21 +0200, and...@2ndquadrant.com wrote:
> >
> > So we're leaking memory here? For --stats that could well be
> > relevant...
> 
> Will fix (I think using a static buffer would be OK here).

In that place, yes. There there's several other places where that's
probably isn't going to work. Probably makes sense to check memory usage
after running it over a larger amount of WAL.

> > It's far from guaranteed that 27 will always suffice. I guess it's ok
> > because it doesn't cause bad breakage, but just some misalignment...
> 
> Yes, that was my thought too.
> 
> I could measure the lengths of things and align columns dynamically, but
> I really doubt it's worth the effort in this case.

Nah. Too much work for no gain ;)

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-07-04 11:34:21 +0200, and...@2ndquadrant.com wrote:
>
> So we're leaking memory here? For --stats that could well be
> relevant...

Will fix (I think using a static buffer would be OK here).

> I think we're going to have to redefine the
> PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT callsite in configure.in […]

OK, will do.

> It's far from guaranteed that 27 will always suffice. I guess it's ok
> because it doesn't cause bad breakage, but just some misalignment...

Yes, that was my thought too.

I could measure the lengths of things and align columns dynamically, but
I really doubt it's worth the effort in this case.

> Many missing spaces here. […]
> Some additional leaking here.

Will fix both.

> Looks like that should at least partially have been in the other
> patch?

Yes, sorry. Will fix.

(I'll set this back to waiting on author. Thanks for having a look.)

-- Abhijit


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

Re: [HACKERS] pg_xlogdump --stats

[Andres Freund]

Hi,

On 2014-07-04 14:42:03 +0530, Abhijit Menon-Sen wrote:
> Sure, attached.
> 
> +const char *
> +heap_identify(uint8 info)
> +{
> + const char *id = NULL;
> +
> + switch (info & XLOG_HEAP_OPMASK)
> + {
> + case XLOG_HEAP_INSERT:
> + id = "INSERT";
> + break;
> + case XLOG_HEAP_DELETE:
> + id = "DELETE";
> + break;
> + case XLOG_HEAP_UPDATE:
> + id = "UPDATE";
> + break;
> + case XLOG_HEAP_HOT_UPDATE:
> + id = "HOT_UPDATE";
> + break;
> + case XLOG_HEAP_NEWPAGE:
> + id = "NEWPAGE";
> + break;
> + case XLOG_HEAP_LOCK:
> + id = "LOCK";
> + break;
> + case XLOG_HEAP_INPLACE:
> + id = "INPLACE";
> + break;
> + }
> +
> + if (info & XLOG_HEAP_INIT_PAGE)
> + id = psprintf("%s+INIT", id);
> +
> + return id;
> +}

So we're leaking memory here? For --stats that could well be relevant...
> +/*
> + * Display a single row of record counts and sizes for an rmgr or record.
> + */
> +static void
> +XLogDumpStatsRow(const char *name,
> +  uint64 n, double n_pct,
> +  uint64 rec_len, double rec_len_pct,
> +  uint64 fpi_len, double fpi_len_pct,
> +  uint64 total_len, double total_len_pct)
> +{
> + printf("%-27s %20zu (%6.02f) %20zu (%6.02f) %20zu (%6.02f) %20zu 
> (%6.02f)\n",
> +name, n, n_pct, rec_len, rec_len_pct, fpi_len, fpi_len_pct,
> +total_len, total_len_pct);
> +}

This (and following locations) is going to break on 32bit platforms. %z
indicates size_t, not 64bit. I think we're going to have to redefine the
PGAC_FUNC_SNPRINTF_LONG_LONG_INT_FORMAT callsite in configure.in to
define INT64_MODIFIER='"ll/l/I64D"'
and then define
#define INT64_FORMAT "%"CppAsString(INT64_MODIFIER)"d"
#define UINT64_FORMAT "%"CppAsString(INT64_MODIFIER)"u"
in c.h based on that, or something like i. This was written blindly, so
it'd might need further work.

Then you can use INT64_MODIFIER in you format strings. Won't be pretty,
but at least it'd work...

> +static void
> +XLogDumpDisplayStats(XLogDumpConfig *config, XLogDumpStats *stats)
> +{

> + /*
> +  * 27 is strlen("Transaction/COMMIT_PREPARED"),
> +  * 20 is strlen(2^64), 8 is strlen("(100.00%)")
> +  */

It's far from guaranteed that 27 will always suffice. I guess it's ok
because it doesn't cause bad breakage, but just some misalignment...

> + for (ri = 0; ri < RM_NEXT_ID; ri++)
> + {
> + uint64  count, rec_len, fpi_len;
> + const RmgrDescData *desc = &RmgrDescTable[ri];
> +
> + if (!config->stats_per_record)
> + {
> + count = stats->rmgr_stats[ri].count;
> + rec_len = stats->rmgr_stats[ri].rec_len;
> + fpi_len = stats->rmgr_stats[ri].fpi_len;
> +
> + XLogDumpStatsRow(desc->rm_name,
> +  count, 
> 100*(double)count/total_count,
> +  rec_len, 
> 100*(double)rec_len/total_rec_len,
> +  fpi_len, 
> 100*(double)fpi_len/total_fpi_len,
> +  rec_len+fpi_len, 
> 100*(double)(rec_len+fpi_len)/total_len);
> + }

Many missing spaces here.

> + else
> + {
> + for (rj = 0; rj < 16; rj++)
> + {
> + const char *id;
> +
> + count = stats->record_stats[ri][rj].count;
> + rec_len = stats->record_stats[ri][rj].rec_len;
> + fpi_len = stats->record_stats[ri][rj].fpi_len;
> +
> + /* Skip undefined combinations and ones that 
> didn't occur */
> + if (count == 0)
> + continue;
> +
> + id = desc->rm_identify(rj << 4);
> + if (id == NULL)
> + id = psprintf("0x%x", rj << 4);
> +
> + XLogDumpStatsRow(psprintf("%s/%s", 
> desc->rm_name, id),
> +  count, 
> 100*(double)count/total_count,
> +  rec_len, 
> 100*(double)rec_len/total_rec_len,
> +  fpi_len, 
> 100*(double)fpi_len/total_fpi_len,
> +   

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-07-04 10:54:08 +0200, and...@2ndquadrant.com wrote:
>
> I'm pretty sure that most committers would want to apply the
> rm_identity part in a separate commit first. Could you make it
> two patches, one introducing rm_identity, and another with
> xlogdump using it?

Sure, attached.

-- Abhijit
diff --git a/src/backend/access/rmgrdesc/clogdesc.c b/src/backend/access/rmgrdesc/clogdesc.c
index e82baa8..08f225d 100644
--- a/src/backend/access/rmgrdesc/clogdesc.c
+++ b/src/backend/access/rmgrdesc/clogdesc.c
@@ -40,3 +40,21 @@ clog_desc(StringInfo buf, XLogRecord *record)
 	else
 		appendStringInfoString(buf, "UNKNOWN");
 }
+
+const char *
+clog_identify(uint8 info)
+{
+	const char *id = NULL;
+
+	switch (info)
+	{
+		case CLOG_ZEROPAGE:
+			id = "ZEROPAGE";
+			break;
+		case CLOG_TRUNCATE:
+			id = "TRUNCATE";
+			break;
+	}
+
+	return id;
+}
diff --git a/src/backend/access/rmgrdesc/dbasedesc.c b/src/backend/access/rmgrdesc/dbasedesc.c
index 0230716..38f3a39 100644
--- a/src/backend/access/rmgrdesc/dbasedesc.c
+++ b/src/backend/access/rmgrdesc/dbasedesc.c
@@ -42,3 +42,21 @@ dbase_desc(StringInfo buf, XLogRecord *record)
 	else
 		appendStringInfoString(buf, "UNKNOWN");
 }
+
+const char *
+dbase_identify(uint8 info)
+{
+	const char *id = NULL;
+
+	switch (info)
+	{
+		case XLOG_DBASE_CREATE:
+			id = "CREATE";
+			break;
+		case XLOG_DBASE_DROP:
+			id = "DROP";
+			break;
+	}
+
+	return id;
+}
diff --git a/src/backend/access/rmgrdesc/gindesc.c b/src/backend/access/rmgrdesc/gindesc.c
index 12d68d7..115ad5b 100644
--- a/src/backend/access/rmgrdesc/gindesc.c
+++ b/src/backend/access/rmgrdesc/gindesc.c
@@ -187,3 +187,45 @@ gin_desc(StringInfo buf, XLogRecord *record)
 			break;
 	}
 }
+
+const char *
+gin_identify(uint8 info)
+{
+	const char *id = NULL;
+
+	switch (info)
+	{
+		case XLOG_GIN_CREATE_INDEX:
+			id = "CREATE_INDEX";
+			break;
+		case XLOG_GIN_CREATE_PTREE:
+			id = "CREATE_PTREE";
+			break;
+		case XLOG_GIN_INSERT:
+			id = "INSERT";
+			break;
+		case XLOG_GIN_SPLIT:
+			id = "SPLIT";
+			break;
+		case XLOG_GIN_VACUUM_PAGE:
+			id = "VACUUM_PAGE";
+			break;
+		case XLOG_GIN_VACUUM_DATA_LEAF_PAGE:
+			id = "VACUUM_DATA_LEAF_PAGE";
+			break;
+		case XLOG_GIN_DELETE_PAGE:
+			id = "DELETE_PAGE";
+			break;
+		case XLOG_GIN_UPDATE_META_PAGE:
+			id = "UPDATE_META_PAGE";
+			break;
+		case XLOG_GIN_INSERT_LISTPAGE:
+			id = "INSERT_LISTPAGE";
+			break;
+		case XLOG_GIN_DELETE_LISTPAGE:
+			id = "DELETE_LISTPAGE";
+			break;
+	}
+
+	return id;
+}
diff --git a/src/backend/access/rmgrdesc/gistdesc.c b/src/backend/access/rmgrdesc/gistdesc.c
index cdac496..e4f288b 100644
--- a/src/backend/access/rmgrdesc/gistdesc.c
+++ b/src/backend/access/rmgrdesc/gistdesc.c
@@ -67,3 +67,24 @@ gist_desc(StringInfo buf, XLogRecord *record)
 			break;
 	}
 }
+
+const char *
+gist_identify(uint8 info)
+{
+	const char *id = NULL;
+
+	switch (info)
+	{
+		case XLOG_GIST_PAGE_UPDATE:
+			id = "PAGE_UPDATE";
+			break;
+		case XLOG_GIST_PAGE_SPLIT:
+			id = "PAGE_SPLIT";
+			break;
+		case XLOG_GIST_CREATE_INDEX:
+			id = "CREATE_INDEX";
+			break;
+	}
+
+	return id;
+}
diff --git a/src/backend/access/rmgrdesc/hashdesc.c b/src/backend/access/rmgrdesc/hashdesc.c
index 86a0376..c58461c 100644
--- a/src/backend/access/rmgrdesc/hashdesc.c
+++ b/src/backend/access/rmgrdesc/hashdesc.c
@@ -20,3 +20,9 @@ void
 hash_desc(StringInfo buf, XLogRecord *record)
 {
 }
+
+const char *
+hash_identify(uint8 info)
+{
+	return NULL;
+}
diff --git a/src/backend/access/rmgrdesc/heapdesc.c b/src/backend/access/rmgrdesc/heapdesc.c
index 7df18fa..e6149be 100644
--- a/src/backend/access/rmgrdesc/heapdesc.c
+++ b/src/backend/access/rmgrdesc/heapdesc.c
@@ -202,3 +202,78 @@ heap2_desc(StringInfo buf, XLogRecord *record)
 	else
 		appendStringInfoString(buf, "UNKNOWN");
 }
+
+const char *
+heap_identify(uint8 info)
+{
+	const char *id = NULL;
+
+	switch (info & XLOG_HEAP_OPMASK)
+	{
+		case XLOG_HEAP_INSERT:
+			id = "INSERT";
+			break;
+		case XLOG_HEAP_DELETE:
+			id = "DELETE";
+			break;
+		case XLOG_HEAP_UPDATE:
+			id = "UPDATE";
+			break;
+		case XLOG_HEAP_HOT_UPDATE:
+			id = "HOT_UPDATE";
+			break;
+		case XLOG_HEAP_NEWPAGE:
+			id = "NEWPAGE";
+			break;
+		case XLOG_HEAP_LOCK:
+			id = "LOCK";
+			break;
+		case XLOG_HEAP_INPLACE:
+			id = "INPLACE";
+			break;
+	}
+
+	if (info & XLOG_HEAP_INIT_PAGE)
+		id = psprintf("%s+INIT", id);
+
+	return id;
+}
+
+const char *
+heap2_identify(uint8 info)
+{
+	const char *id = NULL;
+
+	switch (info & XLOG_HEAP_OPMASK)
+	{
+		case XLOG_HEAP2_CLEAN:
+			id = "CLEAN";
+			break;
+		case XLOG_HEAP2_FREEZE_PAGE:
+			id = "FREEZE_PAGE";
+			break;
+		case XLOG_HEAP2_CLEANUP_INFO:
+			id = "CLEANUP_INFO";
+			break;
+		case XLOG_HEAP2_VISIBLE:
+			id = "VISIBLE";
+			break;
+		case XLOG_HEAP2_MULTI_INSERT:
+			id = "MULTI_INSERT";
+			break;
+		case XLOG_HEAP2_LOCK_UPDATED:
+			id = "LOCK_UPDATED";
+			break;
+		case XLOG_HEAP2_NEW_CID:
+			id = "NEW_CID";
+			break;
+		case XLOG_HEAP2_REWRITE:
+			id =

Re: [HACKERS] pg_xlogdump --stats

[Andres Freund]

On 2014-07-04 14:16:42 +0530, Abhijit Menon-Sen wrote:
> At 2014-07-04 08:38:17 +, dilip.ku...@huawei.com wrote:
> >
> > Once you fix above erros, I think patch is ok from my side.
> 
> I've attached two updated patches here, including the fix to the usage
> message. I'll mark this ready for committer now. (The rm_identify patch
> is posted separately from the xlogdump one only to make the whole thing
> easier to follow.)

I'm pretty sure that most committers would want to apply the rm_identity
part in a separate commit first. Could you make it two patches, one
introducing rm_identity, and another with xlogdump using it?

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-07-04 08:38:17 +, dilip.ku...@huawei.com wrote:
>
> Once you fix above erros, I think patch is ok from my side.

I've attached two updated patches here, including the fix to the usage
message. I'll mark this ready for committer now. (The rm_identify patch
is posted separately from the xlogdump one only to make the whole thing
easier to follow.)

Thank you.

-- Abhijit
diff --git a/contrib/pg_xlogdump/pg_xlogdump.c b/contrib/pg_xlogdump/pg_xlogdump.c
index c555786..239321f 100644
--- a/contrib/pg_xlogdump/pg_xlogdump.c
+++ b/contrib/pg_xlogdump/pg_xlogdump.c
@@ -15,12 +15,28 @@
 #include 
 #include 
 
+#include "access/clog.h"
+#include "access/gin_private.h"
+#include "access/gist_private.h"
+#include "access/heapam_xlog.h"
+#include "access/heapam_xlog.h"
+#include "access/multixact.h"
+#include "access/nbtree.h"
+#include "access/spgist_private.h"
+#include "access/xact.h"
 #include "access/xlog.h"
 #include "access/xlogreader.h"
 #include "access/transam.h"
+#include "catalog/pg_control.h"
+#include "catalog/storage_xlog.h"
+#include "commands/dbcommands.h"
+#include "commands/sequence.h"
+#include "commands/tablespace.h"
 #include "common/fe_memutils.h"
 #include "getopt_long.h"
 #include "rmgrdesc.h"
+#include "storage/standby.h"
+#include "utils/relmapper.h"
 
 
 static const char *progname;
@@ -41,6 +57,8 @@ typedef struct XLogDumpConfig
 	int			stop_after_records;
 	int			already_displayed_records;
 	bool		follow;
+	bool		stats;
+	bool		stats_per_record;
 
 	/* filter options */
 	int			filter_by_rmgr;
@@ -48,6 +66,20 @@ typedef struct XLogDumpConfig
 	bool		filter_by_xid_enabled;
 } XLogDumpConfig;
 
+typedef struct Stats
+{
+	uint64		count;
+	uint64		rec_len;
+	uint64		fpi_len;
+} Stats;
+
+typedef struct XLogDumpStats
+{
+	uint64		count;
+	Stats		rmgr_stats[RM_NEXT_ID];
+	Stats		record_stats[RM_NEXT_ID][16];
+} XLogDumpStats;
+
 static void
 fatal_error(const char *fmt,...)
 __attribute__((format(PG_PRINTF_ATTRIBUTE, 1, 2)));
@@ -322,6 +354,50 @@ XLogDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
 }
 
 /*
+ * Store per-rmgr and per-record statistics for a given record.
+ */
+static void
+XLogDumpCountRecord(XLogDumpConfig *config, XLogDumpStats *stats, XLogRecPtr ReadRecPtr, XLogRecord *record)
+{
+	RmgrId		rmid;
+	uint8  		recid;
+
+	if (config->filter_by_rmgr != -1 &&
+		config->filter_by_rmgr != record->xl_rmid)
+		return;
+
+	if (config->filter_by_xid_enabled &&
+		config->filter_by_xid != record->xl_xid)
+		return;
+
+	stats->count++;
+
+	/* Update per-rmgr statistics */
+
+	rmid = record->xl_rmid;
+
+	stats->rmgr_stats[rmid].count++;
+	stats->rmgr_stats[rmid].rec_len +=
+		record->xl_len + SizeOfXLogRecord;
+	stats->rmgr_stats[rmid].fpi_len +=
+		record->xl_tot_len - (record->xl_len + SizeOfXLogRecord);
+
+	/*
+	 * Update per-record statistics, where the record is identified by a
+	 * combination of the RmgrId and the upper four bits of the xl_info
+	 * field (to give sixteen possible entries per RmgrId).
+	 */
+
+	recid = (record->xl_info & ~XLR_INFO_MASK) >> 4;
+
+	stats->record_stats[rmid][recid].count++;
+	stats->record_stats[rmid][recid].rec_len +=
+		record->xl_len + SizeOfXLogRecord;
+	stats->record_stats[rmid][recid].fpi_len +=
+		record->xl_tot_len - (record->xl_len + SizeOfXLogRecord);
+}
+
+/*
  * Print a record to stdout
  */
 static void
@@ -380,6 +456,498 @@ XLogDumpDisplayRecord(XLogDumpConfig *config, XLogRecPtr ReadRecPtr, XLogRecord
 	}
 }
 
+/*
+ * Given an xl_rmid and the high bits of xl_info, returns a string
+ * describing the record type.
+ */
+static const char *
+identify_record(RmgrId rmid, uint8 info)
+{
+	const RmgrDescData *desc = &RmgrDescTable[rmid];
+	const char *rec;
+
+	rec = psprintf("0x%x", info);
+
+	switch (rmid)
+	{
+		case RM_XLOG_ID:
+			switch (info)
+			{
+case XLOG_CHECKPOINT_SHUTDOWN:
+	rec = "CHECKPOINT_SHUTDOWN";
+	break;
+case XLOG_CHECKPOINT_ONLINE:
+	rec = "CHECKPOINT_ONLINE";
+	break;
+case XLOG_NOOP:
+	rec = "NOOP";
+	break;
+case XLOG_NEXTOID:
+	rec = "NEXTOID";
+	break;
+case XLOG_SWITCH:
+	rec = "SWITCH";
+	break;
+case XLOG_BACKUP_END:
+	rec = "BACKUP_END";
+	break;
+case XLOG_PARAMETER_CHANGE:
+	rec = "PARAMETER_CHANGE";
+	break;
+case XLOG_RESTORE_POINT:
+	rec = "RESTORE_POINT";
+	break;
+case XLOG_FPW_CHANGE:
+	rec = "FPW_CHANGE";
+	break;
+case XLOG_END_OF_RECOVERY:
+	rec = "END_OF_RECOVERY";
+	break;
+case XLOG_FPI:
+	rec = "FPI";
+	break;
+			}
+			break;
+
+		case RM_XACT_ID:
+			switch (info)
+			{
+case XLOG_XACT_COMMIT:
+	rec = "COMMIT";
+	break;
+case XLOG_XACT_PREPARE:
+	rec = "PREPARE";
+	break;
+case XLOG_XACT_ABORT:
+	rec = "ABORT";
+	break;
+case XLOG_XACT_COMMIT_PREPARED:
+	rec = "COMMIT_PREPARED";
+	break;
+case XLOG_XACT_ABORT_PREPARED:
+	rec = "ABORT_PREPARED";
+	break;
+case XLOG_XA

Re: [HACKERS] pg_xlogdump --stats

[Dilip kumar]

On 04 July 2014 12:07, Abhijit Menon-Sen Wrote,

> -Original Message-
> From: Abhijit Menon-Sen [mailto:a...@2ndquadrant.com]
> Sent: 04 July 2014 12:07
> To: Dilip kumar
> Cc: pgsql-hackers@postgresql.org; furu...@pm.nttdata.co.jp
> Subject: Re: [HACKERS] pg_xlogdump --stats
> 
> At 2014-06-30 05:19:10 +, dilip.ku...@huawei.com wrote:
> >
> > Please fix these issues and send the updated patch..
> >
> > I will continue reviewing the patch..
> 
> Did you get anywhere with the updated patch?
> 

Patch looks fine to me, except few small comments.

1. Update this new option in "usage" function also  this still have the old way 
{ -z, --stats[=record] }

{"stats", no_argument, NULL, 'z'},
{"record-stats", no_argument, NULL, 'Z'},

2. While applying stats-newopt.dif (after applying stat2.diff), some error in 
merging sgml file.

patching file `doc/src/sgml/pg_xlogdump.sgml'
Hunk #1 FAILED at 181.
1 out of 1 hunk FAILED -- saving rejects to doc/src/sgml/pg_xlogdump.sgml.rej

Once you fix above erros, I think patch is ok from my side.

Thanks & Regards,
Dilip Kumar



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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-06-30 05:19:10 +, dilip.ku...@huawei.com wrote:
>
> Please fix these issues and send the updated patch..
> 
> I will continue reviewing the patch..

Did you get anywhere with the updated patch?

-- Abhijit


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

Re: [HACKERS] pg_xlogdump --stats

[Craig Ringer]

On 07/02/2014 09:20 AM, Marti Raudsepp wrote:

> You might also add units (kB/MB) to the table like pg_size_pretty,
> although that would make the magnitudes harder to gauge.

What 'du' does, or a subset of, may be worthwhile.

-k: kB
-b: blocks
-m: MB
-h: human-readable auto-scaling

though I think just the "-h" flag would be useful here, meaning "wrap in
pg_size_pretty".

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-07-02 04:20:31 +0300, ma...@juffo.org wrote:
>
> As far as functionality goes, it does exactly what I needed it to do;
> the output is very clear.

Good to hear.

> You might also add units (kB/MB) to the table like pg_size_pretty,
> although that would make the magnitudes harder to gauge.

I think df-style -k/m/g switches might be useful to scale all printed
values. If we did that, we could also shrink the columns accordingly.
But that would also complicate the code a bit. I think it's better to
start with the simplest thing, and add more tweaks later.

-- Abhijit


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

Re: [HACKERS] pg_xlogdump --stats

[Marti Raudsepp]

On Tue, Jul 1, 2014 at 11:13 PM, Abhijit Menon-Sen  wrote:
> In CF terms, did you form any opinion while porting the patch I posted
> about whether it's sensible/ready for inclusion in 9.5?

I didn't look at the code more than necessary to make the build work.

As far as functionality goes, it does exactly what I needed it to do;
the output is very clear. I wanted to see how much WAL is being
generated from SELECT FOR UPDATE locks.

Here are some thoughts:

The "FPI" acronym made me wonder at first. Perhaps "Full page image
size" would be clearer (exactly 20 characters long, so it fits).

But on the other hand, I find that the table is too wide, I always
need to stretch my terminal window to fit it. I don't think we need to
accommodate for 10^20 bytes ~ 87 exabytes of WAL files. :)

You might also add units (kB/MB) to the table like pg_size_pretty,
although that would make the magnitudes harder to gauge.

> P.S. In your patch, the documentation changes are duplicated.

Oh right you are, I don't know how that happened. I also missed some
record types (Btree/0x80, Btree/0xb0).

Regards,
Marti


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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-07-01 16:39:57 +0300, ma...@juffo.org wrote:
>
> > Here's a patch to make pg_xlogdump print summary statistics instead
> > of individual records.
> 
> Thanks! I had a use for this feature so I backported the (first) patch
> to PostgreSQL 9.3. It's a rush job so it's ugly and may have bugs, but
> it works for me. Just posting here, maybe someone else will find it
> useful too.

Thanks for taking the trouble.

In CF terms, did you form any opinion while porting the patch I posted
about whether it's sensible/ready for inclusion in 9.5?

-- Abhijit

P.S. In your patch, the documentation changes are duplicated.


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

Re: [HACKERS] pg_xlogdump --stats

[Marti Raudsepp]

On Wed, Jun 4, 2014 at 1:47 PM, Abhijit Menon-Sen  wrote:
> Here's a patch to make pg_xlogdump print summary statistics instead of
> individual records.

Thanks! I had a use for this feature so I backported the (first) patch
to PostgreSQL 9.3. It's a rush job so it's ugly and may have bugs, but
it works for me. Just posting here, maybe someone else will find it
useful too.

Regards,
Marti


xlogdiff_stats_93.patch.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-06-30 12:05:06 +0530, a...@2ndquadrant.com wrote:
>
> It may be that the best thing to do would be to avoid using
> optional_argument altogether, and have separate --stats and
> --stats-per-record options. Thoughts?

That's what I've done in the attached patch, except I've called the new
option --record-stats. Both options now use no_argument. This should
apply on top of the diff I posted a little while ago.

-- Abhijit
commit cc9422aa71ef0b507c634282272be3fd15c39c0b
Author: Abhijit Menon-Sen 
Date:   Mon Jun 30 12:15:54 2014 +0530

Introduce --record-stats to avoid use of optional_argument

diff --git a/contrib/pg_xlogdump/pg_xlogdump.c b/contrib/pg_xlogdump/pg_xlogdump.c
index 47838d4..1853b47 100644
--- a/contrib/pg_xlogdump/pg_xlogdump.c
+++ b/contrib/pg_xlogdump/pg_xlogdump.c
@@ -609,7 +609,8 @@ main(int argc, char **argv)
 		{"timeline", required_argument, NULL, 't'},
 		{"xid", required_argument, NULL, 'x'},
 		{"version", no_argument, NULL, 'V'},
-		{"stats", optional_argument, NULL, 'z'},
+		{"stats", no_argument, NULL, 'z'},
+		{"record-stats", no_argument, NULL, 'Z'},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -643,7 +644,7 @@ main(int argc, char **argv)
 		goto bad_argument;
 	}
 
-	while ((option = getopt_long(argc, argv, "be:?fn:p:r:s:t:Vx:z::",
+	while ((option = getopt_long(argc, argv, "be:?fn:p:r:s:t:Vx:zZ",
  long_options, &optindex)) != -1)
 	{
 		switch (option)
@@ -738,18 +739,10 @@ main(int argc, char **argv)
 break;
 			case 'z':
 config.stats = true;
-config.stats_per_record = false;
-if (optarg)
-{
-	if (strcmp(optarg, "record") == 0)
-		config.stats_per_record = true;
-	else if (strcmp(optarg, "rmgr") != 0)
-	{
-		fprintf(stderr, "%s: unrecognised argument to --stats: %s\n",
-progname, optarg);
-		goto bad_argument;
-	}
-}
+break;
+			case 'Z':
+config.stats = true;
+config.stats_per_record = true;
 break;
 			default:
 goto bad_argument;
diff --git a/doc/src/sgml/pg_xlogdump.sgml b/doc/src/sgml/pg_xlogdump.sgml
index d9f4a6a..bfd9eb9 100644
--- a/doc/src/sgml/pg_xlogdump.sgml
+++ b/doc/src/sgml/pg_xlogdump.sgml
@@ -181,12 +181,22 @@ PostgreSQL documentation
 
  
   -z
-  --stats[=record]
+  --stats
   

 Display summary statistics (number and size of records and
-full-page images) instead of individual records. Optionally
-generate statistics per-record instead of per-rmgr.
+full-page images per rmgr) instead of individual records.
+   
+  
+ 
+
+ 
+  -Z
+  --record-stats
+  
+   
+Display summary statistics (number and size of records and
+full-page images) instead of individual records.

   
  

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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-06-30 05:19:10 +, dilip.ku...@huawei.com wrote:
>
> I have started reviewing the patch..

Thanks.

> 1. Patch applied to git head cleanly.
> 2. Compiled in Linux  -- Some warnings same as mentioned by furuyao 

I've attached an updated version of the patch which should fix the
warnings by using %zu.

> 3. Some compilation error in windows
> .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2065: 'optional_argument' 
> : undeclared identifier
> .\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2099: initializer is not a 
> constant
> 
> optional_argument should be added to getopt_long.h file for windows.

Hmm. I have no idea what to do about this. I did notice when I wrote the
code that nothing else used optional_argument, but I didn't realise that
it wouldn't work on Windows.

It may be that the best thing to do would be to avoid using
optional_argument altogether, and have separate --stats and
--stats-per-record options. Thoughts?

> Please fix these issues and send the updated patch..

I've also attached a separate patch to factor out the identify_record
function into rm_identify functions in the individual rmgr files, so
that the code can live next to the respective _desc functions.

This allows us to remove a number of #includes from pg_xlogdump, and
makes the code a bit more straightforward to read.

-- Abhijit
diff --git a/contrib/pg_xlogdump/pg_xlogdump.c b/contrib/pg_xlogdump/pg_xlogdump.c
index 824b8c3..1d3e664 100644
--- a/contrib/pg_xlogdump/pg_xlogdump.c
+++ b/contrib/pg_xlogdump/pg_xlogdump.c
@@ -15,12 +15,28 @@
 #include 
 #include 
 
+#include "access/clog.h"
+#include "access/gin_private.h"
+#include "access/gist_private.h"
+#include "access/heapam_xlog.h"
+#include "access/heapam_xlog.h"
+#include "access/multixact.h"
+#include "access/nbtree.h"
+#include "access/spgist_private.h"
+#include "access/xact.h"
 #include "access/xlog.h"
 #include "access/xlogreader.h"
 #include "access/transam.h"
+#include "catalog/pg_control.h"
+#include "catalog/storage_xlog.h"
+#include "commands/dbcommands.h"
+#include "commands/sequence.h"
+#include "commands/tablespace.h"
 #include "common/fe_memutils.h"
 #include "getopt_long.h"
 #include "rmgrdesc.h"
+#include "storage/standby.h"
+#include "utils/relmapper.h"
 
 
 static const char *progname;
@@ -41,6 +57,8 @@ typedef struct XLogDumpConfig
 	int			stop_after_records;
 	int			already_displayed_records;
 	bool		follow;
+	bool		stats;
+	bool		stats_per_record;
 
 	/* filter options */
 	int			filter_by_rmgr;
@@ -48,6 +66,20 @@ typedef struct XLogDumpConfig
 	bool		filter_by_xid_enabled;
 } XLogDumpConfig;
 
+typedef struct Stats
+{
+	uint64		count;
+	uint64		rec_len;
+	uint64		fpi_len;
+} Stats;
+
+typedef struct XLogDumpStats
+{
+	uint64		count;
+	Stats		rmgr_stats[RM_NEXT_ID];
+	Stats		record_stats[RM_NEXT_ID][16];
+} XLogDumpStats;
+
 static void
 fatal_error(const char *fmt,...)
 __attribute__((format(PG_PRINTF_ATTRIBUTE, 1, 2)));
@@ -322,6 +354,39 @@ XLogDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
 }
 
 /*
+ * Store per-rmgr and per-record statistics for a given record.
+ */
+static void
+XLogDumpCountRecord(XLogDumpConfig *config, XLogDumpStats *stats, XLogRecPtr ReadRecPtr, XLogRecord *record)
+{
+	RmgrId		rmid;
+	uint8  		recid;
+
+	if (config->filter_by_rmgr != -1 &&
+		config->filter_by_rmgr != record->xl_rmid)
+		return;
+
+	if (config->filter_by_xid_enabled &&
+		config->filter_by_xid != record->xl_xid)
+		return;
+
+	rmid = record->xl_rmid;
+	recid = (record->xl_info & ~XLR_INFO_MASK) >> 4;
+
+	stats->count++;
+
+	stats->rmgr_stats[rmid].count++;
+	stats->rmgr_stats[rmid].rec_len += record->xl_len;
+	stats->rmgr_stats[rmid].fpi_len +=
+		(record->xl_tot_len - record->xl_len - SizeOfXLogRecord);
+
+	stats->record_stats[rmid][recid].count++;
+	stats->record_stats[rmid][recid].rec_len += record->xl_len;
+	stats->record_stats[rmid][recid].fpi_len +=
+		(record->xl_tot_len - record->xl_len - SizeOfXLogRecord);
+}
+
+/*
  * Print a record to stdout
  */
 static void
@@ -380,6 +445,476 @@ XLogDumpDisplayRecord(XLogDumpConfig *config, XLogRecPtr ReadRecPtr, XLogRecord
 	}
 }
 
+/*
+ * Given an xl_rmid and the high bits of xl_info, returns a string
+ * describing the record type.
+ */
+static const char *
+identify_record(RmgrId rmid, uint8 info)
+{
+	const RmgrDescData *desc = &RmgrDescTable[rmid];
+	const char *rec;
+
+	rec = psprintf("0x%x", info);
+
+	switch (rmid)
+	{
+		case RM_XLOG_ID:
+			switch (info)
+			{
+case XLOG_CHECKPOINT_SHUTDOWN:
+	rec = "CHECKPOINT_SHUTDOWN";
+	break;
+case XLOG_CHECKPOINT_ONLINE:
+	rec = "CHECKPOINT_ONLINE";
+	break;
+case XLOG_NOOP:
+	rec = "NOOP";
+	break;
+case XLOG_NEXTOID:
+	rec = "NEXTOID";
+	break;
+case XLOG_SWITCH:
+	rec = "SWITCH";
+	break;
+case XLOG_BACKUP_END:
+	rec = "BACKUP_END";
+	break;
+case XLOG_PARAMETER_CHANGE:
+	rec = "PARAMETER_CHANGE";
+	break;
+case XLOG_RESTOR

Re: [HACKERS] pg_xlogdump --stats

[Dilip kumar]

On 13 June 2014 13:01, Abhijit Menon-Sen Wrote

> 
> I've changed this to use %zu at Álvaro's suggestion. I'll post an
> updated patch after I've finished some (unrelated) refactoring.

I have started reviewing the patch..

1. Patch applied to git head cleanly.
2. Compiled in Linux  -- Some warnings same as mentioned by furuyao 

3. Some compilation error in windows
.\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2065: 'optional_argument' : 
undeclared identifier
.\contrib\pg_xlogdump\pg_xlogdump.c(1002) : error C2099: initializer is not a 
constant

optional_argument should be added to getopt_long.h file for windows.

Please fix these issues and send the updated patch..

I will continue reviewing the patch..


Regards,
Dilip




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

Re: [HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

At 2014-06-10 18:04:13 +0900, furu...@pm.nttdata.co.jp wrote:
>
> The function works fine. It is a good to the learning of PostgreSQL.

Thanks for having a look.

> pg_xlogdump.c: In function ‘XLogDumpStatsRow’:
> pg_xlogdump.c:851: warning: format ‘%20llu’ expects type ‘long long unsigned 
> int’, but argument 3 has type ‘uint64’

I've changed this to use %zu at Álvaro's suggestion. I'll post an
updated patch after I've finished some (unrelated) refactoring.

-- Abhijit


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

Re: [HACKERS] pg_xlogdump --stats

[furuyao]

> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Abhijit
> Menon-Sen
> Sent: Wednesday, June 04, 2014 7:47 PM
> To: pgsql-hackers@postgresql.org
> Subject: [HACKERS] pg_xlogdump --stats
> 
> Hi.
> 
> Here's a patch to make pg_xlogdump print summary statistics instead of
> individual records.
> 
The function works fine. It is a good to the learning of PostgreSQL.
But By applying the patch,warning comes out then make.
Although I think that's no particular problem, However I think better to fix is 
good.


$ make
...
pg_xlogdump.c: In function ‘XLogDumpStatsRow’:
pg_xlogdump.c:851: warning: format ‘%20llu’ expects type ‘long long unsigned 
int’, but argument 3 has type ‘uint64’
pg_xlogdump.c:851: warning: format ‘%20llu’ expects type ‘long long unsigned 
int’, but argument 5 has type ‘uint64’
pg_xlogdump.c:851: warning: format ‘%20llu’ expects type ‘long long unsigned 
int’, but argument 7 has type ‘uint64’
pg_xlogdump.c:851: warning: format ‘%20llu’ expects type ‘long long unsigned 
int’, but argument 9 has type ‘uint64’
pg_xlogdump.c: In function ‘XLogDumpDisplayStats’:
pg_xlogdump.c:948: warning: format ‘%20llu’ expects type ‘long long unsigned 
int’, but argument 3 has type ‘uint64’
pg_xlogdump.c:948: warning: format ‘%20llu’ expects type ‘long long unsigned 
int’, but argument 5 has type ‘uint64’
pg_xlogdump.c:948: warning: format ‘%20llu’ expects type ‘long long unsigned 
int’, but argument 7 has type ‘uint64’
pg_xlogdump.c:948: warning: format ‘%20llu’ expects type ‘long long unsigned 
int’, but argument 9 has type ‘uint64'
...

My environment:
RHEL 6.4
gcc version 4.4.7 20120313 (Red Hat 4.4.7-3) (GCC)

Regards,

-- 
Furuya Osamu


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

[HACKERS] pg_xlogdump --stats

[Abhijit Menon-Sen]

Hi.

Here's a patch to make pg_xlogdump print summary statistics instead of
individual records.

By default, for each rmgr it shows the number of records, the size of
rmgr-specific records, the size of full-page images, and the combined
size. With --stats=record it shows these figures for each rmgr/xl_info
combination (omitting those with zero counts for readability).

Here's an example of the output after a few pgbench runs with the
default checkpoint settings. I raised wal_keep_segments, resulting
in 3.5GB of WAL data in pg_xlog.

As you can see in the "Total" line, 96.83% of this is full-page images.
As Andres observed, this is a good demonstration of why one should not
use the default checkpoint_segments in production.

$ ../bin/pg_xlogdump --stats=record 00010001 
000100DE
Type   N  (%)  Record size  
(%) FPI size  (%)Combined size  (%)
   -  ---  ---  
---   ----  ---
XLOG/CHECKPOINT_SHUTDOWN  16 (  0.00) 1152 
(  0.00)0 (  0.00) 1152 (  0.00)
XLOG/CHECKPOINT_ONLINE80 (  0.00) 5760 
(  0.01)0 (  0.00) 5760 (  0.00)
XLOG/NEXTOID  12 (  0.00)   48 
(  0.00)0 (  0.00)   48 (  0.00)
Transaction/COMMIT71 (  0.00) 4708 
(  0.00)0 (  0.00) 4708 (  0.00)
Transaction/COMMIT_COMPACT413956 ( 14.82)  4967472 
(  4.35)0 (  0.00)  4967472 (  0.14)
Storage/CREATE   231 (  0.01) 3696 
(  0.00)0 (  0.00) 3696 (  0.00)
Storage/TRUNCATE   1 (  0.00)   16 
(  0.00)0 (  0.00)   16 (  0.00)
CLOG/ZEROPAGE 13 (  0.00)   52 
(  0.00)0 (  0.00)   52 (  0.00)
Database/CREATE3 (  0.00)   48 
(  0.00)0 (  0.00)   48 (  0.00)
RelMap/UPDATE 14 (  0.00) 7336 
(  0.01)0 (  0.00) 7336 (  0.00)
Heap2/CLEAN   369312 ( 13.22) 10769122 
(  9.43)   2698910088 ( 77.33)   2709679210 ( 75.17)
Heap2/FREEZE_PAGE 53 (  0.00) 3276 
(  0.00)   327732 (  0.01)   331008 (  0.01)
Heap2/VISIBLE  58160 (  2.08)  1163200 
(  1.02)   599768 (  0.02)  1762968 (  0.05)
Heap2/MULTI_INSERT 1 (  0.00)   59 
(  0.00)0 (  0.00)   59 (  0.00)
Heap2/MULTI_INSERT+INIT7 (  0.00)38874 
(  0.03)0 (  0.00)38874 (  0.00)
Heap/INSERT   425472 ( 15.23) 22392664 
( 19.61)  6081712 (  0.17) 28474376 (  0.79)
Heap/DELETE 1638 (  0.06)42588 
(  0.04)19800 (  0.00)62388 (  0.00)
Heap/UPDATE53912 (  1.93)  7145531 
(  6.26)390264760 ( 11.18)397410291 ( 11.03)
Heap/HOT_UPDATE  1185607 ( 42.43) 59538947 
( 52.13) 48724168 (  1.40)108263115 (  3.00)
Heap/LOCK 199320 (  7.13)  4983000 
(  4.36)  1656812 (  0.05)  6639812 (  0.18)
Heap/INPLACE 469 (  0.02)66676 
(  0.06)   558604 (  0.02)   625280 (  0.02)
Heap/INSERT+INIT2992 (  0.11)   272895 
(  0.24)0 (  0.00)   272895 (  0.01)
Heap/UPDATE+INIT1184 (  0.04)   146420 
(  0.13)  6611352 (  0.19)  6757772 (  0.19)
Btree/INSERT_LEAF  81058 (  2.90)  2224916 
(  1.95)336444372 (  9.64)338669288 (  9.40)
Btree/INSERT_UPPER   128 (  0.00) 5272 
(  0.00)16368 (  0.00)21640 (  0.00)
Btree/SPLIT_L 48 (  0.00)   171384 
(  0.15)46712 (  0.00)   218096 (  0.01)
Btree/

Re: [HACKERS] pg_xlogdump compiler warning

[Andres Freund]

On 2013-02-26 15:52:01 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2013-02-25 18:15:48 -0500, Peter Eisentraut wrote:
> >> compat.c: In function ‘timestamptz_to_str’:
> >> compat.c:56:9: error: passing argument 1 of ‘localtime’ from incompatible 
> >> pointer type [-Werror]
> >> In file included from compat.c:21:0:
> >> /usr/include/time.h:237:19: note: expected ‘const time_t *’ but argument 
> >> is of type ‘pg_time_t *’
> 
> > 32bit I guess?
> 
> Yeah, I see the same on a 32-bit machine.
> 
> > A simple cast should do for now. Patch attached.
> 
> Pushed along with some other, more cosmetic cleanups.

Thanks. The statics's et al were stupid, my fault.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump compiler warning

[Tom Lane]

Andres Freund  writes:
> On 2013-02-25 18:15:48 -0500, Peter Eisentraut wrote:
>> compat.c: In function ‘timestamptz_to_str’:
>> compat.c:56:9: error: passing argument 1 of ‘localtime’ from 
>> incompatible pointer type [-Werror]
>> In file included from compat.c:21:0:
>> /usr/include/time.h:237:19: note: expected ‘const time_t *’ but argument 
>> is of type ‘pg_time_t *’

> 32bit I guess?

Yeah, I see the same on a 32-bit machine.

> A simple cast should do for now. Patch attached.

Pushed along with some other, more cosmetic cleanups.

regards, tom lane


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

Re: [HACKERS] pg_xlogdump

[Robert Haas]

On Tue, Feb 26, 2013 at 12:02 PM, Peter Eisentraut  wrote:
> On 2/26/13 11:45 AM, Tom Lane wrote:
>> But let's not break the cases that do work.  One
>> of the functions of contrib/ is to serve as models/skeletons for
>> external modules.  If we pull out the "useless" PGXS support then we'll
>> just be making it that much harder to copy a contrib module and start
>> doing something useful.
>
> Well, this is exactly the problem.  Because of this skeleton idea, most
> external extension modules do not build unless you set USE_PGXS=1 before
> building, because they think that they live in contrib by default, which
> is completely bizarre and user-unfriendly.
>
> We could have an actual example or skeleton whose purpose is to teach
> extension authors.  The actual contrib module makefiles should just do
> their job and don't pretend to teach things that are misguided and/or
> don't work.

I couldn't agree more.  I can't think how much time I've wasted by
forgetting (or remembering) to type USE_PGXS=1 over the years, or not
realizing that I needed to.  I'm sure there are many other people in
the same boat.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] pg_xlogdump

[Peter Geoghegan]

On 26 February 2013 17:02, Peter Eisentraut  wrote:
> Well, this is exactly the problem.  Because of this skeleton idea, most
> external extension modules do not build unless you set USE_PGXS=1 before
> building, because they think that they live in contrib by default, which
> is completely bizarre and user-unfriendly.

repmgr is a popular external extension module. The README actually
suggests moving the entire source tree into /contrib as an alternative
to setting USE_PGXS=1. That does seem kind of weird, and yet I can
understand the train of thought.

My advice to others working on external modules would not be to
generalise from the example of /contrib, but to generalise from the
example of popular external modules. This is particularly important
when targeting multiple Postgres versions.


-- 
Regards,
Peter Geoghegan


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

Re: [HACKERS] pg_xlogdump

[Peter Eisentraut]

On 2/26/13 11:45 AM, Tom Lane wrote:
> But let's not break the cases that do work.  One
> of the functions of contrib/ is to serve as models/skeletons for
> external modules.  If we pull out the "useless" PGXS support then we'll
> just be making it that much harder to copy a contrib module and start
> doing something useful.

Well, this is exactly the problem.  Because of this skeleton idea, most
external extension modules do not build unless you set USE_PGXS=1 before
building, because they think that they live in contrib by default, which
is completely bizarre and user-unfriendly.

We could have an actual example or skeleton whose purpose is to teach
extension authors.  The actual contrib module makefiles should just do
their job and don't pretend to teach things that are misguided and/or
don't work.



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

Re: [HACKERS] pg_xlogdump

[Dimitri Fontaine]

Andres Freund  writes:
> Wait what? So I need to make install before I can compile extensions?
> That doesn't seem to be something realistic.

You know, any extension that's not in our source tree out there is
maintained in a way to target all supported versions of PostgreSQL from
the same sources. I understand that we want to continue applying our
versioning rules to contribs, and I think that it's a good idea.

But now that we're making a real cut decision about that, I think
contribs should be documented as NOT extensions.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


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

Re: [HACKERS] pg_xlogdump

[Tom Lane]

Andres Freund  writes:
> Well, it is broken for xlogdump because it needs a sourcetree arround.

> All I personally really want to do is to replace the usual stanza for
> pg_xlogdump with something like:

> ifdef USE_PGXS
> $(error Building pg_xlogdump with PGXS is not supported)
> else
> ...

Seems reasonable.  But let's not break the cases that do work.  One
of the functions of contrib/ is to serve as models/skeletons for
external modules.  If we pull out the "useless" PGXS support then we'll
just be making it that much harder to copy a contrib module and start
doing something useful.

regards, tom lane


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

Re: [HACKERS] pg_xlogdump

[Andres Freund]

On 2013-02-26 11:33:48 -0500, Tom Lane wrote:
> [ Sigh... ]  Why this eagerness to fix what isn't broken?

Well, it is broken for xlogdump because it needs a sourcetree arround.

All I personally really want to do is to replace the usual stanza for
pg_xlogdump with something like:

ifdef USE_PGXS
$(error Building pg_xlogdump with PGXS is not supported)
include $(PGXS)
else
...

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump

[Andres Freund]

On 2013-02-26 17:23:01 +0100, Dimitri Fontaine wrote:
> Peter Eisentraut  writes:
> > I for one wonder why we even have PGXS support in contrib at all.  It's
> > not documented or tested anywhere, so it might as well not exist.

Agreed. I personally think we just should build contrib/ as part of the
normal build and be done with it.

There's currently the argument that building them separately is
interesting because you can do it from a separate sourcetree if you have
a precompiled postgres around. But whats the usecase for that?

I would much rather designate one example module that is always built
with PGXS (possibly only during regress's install?), so we reliably test that.

> I think I did about the same comment back when cooking the extension
> patch, and the answer then was all about providing PGXS usage examples.
> Now if none of the buildfarm animals are actually building our contribs
> out of tree, maybe we should just remove those examples.
> 
> The cost of keeping them is that they double-up the Makefile content and
> lots of users do think they need their extension's Makefile to be
> structured the same. The common effect before the extension availability
> was for people to provide extensions that would only build in tree.
> 
> I don't want to kill cleaning up those Makefiles, but I still want to
> make a strong correlation in between that point and providing core
> maintained extensions. I don't think extensions should have support for
> being built in-tree at all.

Wait what? So I need to make install before I can compile extensions?
That doesn't seem to be something realistic.

> My proposal: paint them extension rather than contrib modules, then
> cleanup Makefiles so as to stop building them in-tree.

Imo painting them extensions is something unrelated. Which doesn't apply
to everything in contrib/, there are several modules that don't make
sense as extensions (chkpass, oid2name, passwordcheck,
pg_archivecleanup, pgbench, ...).

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump

[Dimitri Fontaine]

Tom Lane  writes:
> work today --- in particular, this proposal will break building contrib
> before the main tree has been installed.

True that.

> If somebody wants to set up a buildfarm member that occasionally tests
> PGXS building of contrib/, that's fine with me.  But it isn't, and never
> will be, the main build scenario for contrib/ IMO.

I guess you just made a final vote on the whole idea of making contrib a
set of extensions maintained by the PostgreSQL commiters, then. It might
be good to document that position, so that the topic isn't raised again
each year?

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


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

Re: [HACKERS] pg_xlogdump compile error

[Fujii Masao]

On Wed, Feb 27, 2013 at 1:25 AM, Andres Freund  wrote:
> Hi,
>
> On 2013-02-27 00:34:54 +0900, Fujii Masao wrote:
>> Hi,
>>
>> When I compiled pg_xlogdump in HEAD, I got the following error.
>>
>> make: *** No rule to make target `rmgrdesc.o', needed by `pg_xlogdump'.  
>> Stop.
>>
>> $ uname -a
>> Darwin hrk.local 11.4.2 Darwin Kernel Version 11.4.2: Thu Aug 23
>> 16:25:48 PDT 2012; root:xnu-1699.32.7~1/RELEASE_X86_64 x86_64
>
> Is that with a clean tree? Or are you perhaps influenced by the bug
> fixed in e5bf0c376ed43feaebbe37519a6b8bc8e795f1d2? I.e. that make clean
> removed rmgrdesc.c?

Thanks! You're right.

rmgrdesc.c was accidentally deleted and I could not notice that.

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] pg_xlogdump

[Tom Lane]

Dimitri Fontaine  writes:
> Peter Eisentraut  writes:
>> I for one wonder why we even have PGXS support in contrib at all.  It's
>> not documented or tested anywhere, so it might as well not exist.

> I think I did about the same comment back when cooking the extension
> patch, and the answer then was all about providing PGXS usage examples.
> Now if none of the buildfarm animals are actually building our contribs
> out of tree, maybe we should just remove those examples.

> The cost of keeping them is that they double-up the Makefile content and
> lots of users do think they need their extension's Makefile to be
> structured the same. The common effect before the extension availability
> was for people to provide extensions that would only build in tree.

> I don't want to kill cleaning up those Makefiles, but I still want to
> make a strong correlation in between that point and providing core
> maintained extensions. I don't think extensions should have support for
> being built in-tree at all.

> My proposal: paint them extension rather than contrib modules, then
> cleanup Makefiles so as to stop building them in-tree.

[ Sigh... ]  Why this eagerness to fix what isn't broken?

Leave the Makefiles alone.  They're not broken and they provide useful
examples, plus a sense of continuity between in-tree and not-in-tree
extensions.  Any change here will likely break build scenarios that
work today --- in particular, this proposal will break building contrib
before the main tree has been installed.

If somebody wants to set up a buildfarm member that occasionally tests
PGXS building of contrib/, that's fine with me.  But it isn't, and never
will be, the main build scenario for contrib/ IMO.

regards, tom lane


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

Re: [HACKERS] pg_xlogdump compile error

[Andres Freund]

Hi,

On 2013-02-27 00:34:54 +0900, Fujii Masao wrote:
> Hi,
> 
> When I compiled pg_xlogdump in HEAD, I got the following error.
> 
> make: *** No rule to make target `rmgrdesc.o', needed by `pg_xlogdump'.  Stop.
> 
> $ uname -a
> Darwin hrk.local 11.4.2 Darwin Kernel Version 11.4.2: Thu Aug 23
> 16:25:48 PDT 2012; root:xnu-1699.32.7~1/RELEASE_X86_64 x86_64

Is that with a clean tree? Or are you perhaps influenced by the bug
fixed in e5bf0c376ed43feaebbe37519a6b8bc8e795f1d2? I.e. that make clean
removed rmgrdesc.c?

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump

[Dimitri Fontaine]

Peter Eisentraut  writes:
> I for one wonder why we even have PGXS support in contrib at all.  It's
> not documented or tested anywhere, so it might as well not exist.

I think I did about the same comment back when cooking the extension
patch, and the answer then was all about providing PGXS usage examples.
Now if none of the buildfarm animals are actually building our contribs
out of tree, maybe we should just remove those examples.

The cost of keeping them is that they double-up the Makefile content and
lots of users do think they need their extension's Makefile to be
structured the same. The common effect before the extension availability
was for people to provide extensions that would only build in tree.

I don't want to kill cleaning up those Makefiles, but I still want to
make a strong correlation in between that point and providing core
maintained extensions. I don't think extensions should have support for
being built in-tree at all.

My proposal: paint them extension rather than contrib modules, then
cleanup Makefiles so as to stop building them in-tree.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


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

[HACKERS] pg_xlogdump compile error

[Fujii Masao]

Hi,

When I compiled pg_xlogdump in HEAD, I got the following error.

make: *** No rule to make target `rmgrdesc.o', needed by `pg_xlogdump'.  Stop.

$ uname -a
Darwin hrk.local 11.4.2 Darwin Kernel Version 11.4.2: Thu Aug 23
16:25:48 PDT 2012; root:xnu-1699.32.7~1/RELEASE_X86_64 x86_64

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] pg_xlogdump compiler warning

[Andres Freund]

Hi,

On 2013-02-25 18:15:48 -0500, Peter Eisentraut wrote:
> compat.c: In function ‘timestamptz_to_str’:
> compat.c:56:9: error: passing argument 1 of ‘localtime’ from incompatible 
> pointer type [-Werror]
> In file included from compat.c:21:0:
> /usr/include/time.h:237:19: note: expected ‘const time_t *’ but argument is 
> of type ‘pg_time_t *’
> 
> gcc 4.7.2

32bit I guess?

A simple cast should do for now. Patch attached.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/contrib/pg_xlogdump/compat.c b/contrib/pg_xlogdump/compat.c
index 99ecad7..0aee756 100644
--- a/contrib/pg_xlogdump/compat.c
+++ b/contrib/pg_xlogdump/compat.c
@@ -52,7 +52,8 @@ timestamptz_to_str(TimestampTz dt)
 	static char buf[MAXDATELEN + 1];
 	static char ts[MAXDATELEN + 1];
 	static char zone[MAXDATELEN + 1];
-	pg_time_t	result = timestamptz_to_time_t(dt);
+	/* cast to time_t so we can use localtime() */
+	time_t	result = (time_t)timestamptz_to_time_t(dt);
 	struct tm  *ltime = localtime(&result);
 
 	strftime(ts, sizeof(zone), "%Y-%m-%d %H:%M:%S", ltime);

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

[HACKERS] pg_xlogdump compiler warning

[Peter Eisentraut]

compat.c: In function ‘timestamptz_to_str’:
compat.c:56:9: error: passing argument 1 of ‘localtime’ from incompatible 
pointer type [-Werror]
In file included from compat.c:21:0:
/usr/include/time.h:237:19: note: expected ‘const time_t *’ but argument is of 
type ‘pg_time_t *’

gcc 4.7.2




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

Re: [HACKERS] pg_xlogdump

[Peter Eisentraut]

On Sun, 2013-02-24 at 09:34 -0500, Tom Lane wrote:
> > I independently wonder whether we should remove the PGXS stub from
> > xlogdump, given it relies on a full sourcetree available?
> 
> I'd just as soon keep its Makefile looking like all the others.
> 
If the code doesn't actually work, it should be deleted.  There is no
value in keeping the Makefile looking like the others, because it
doesn't in fact look like the others.

I for one wonder why we even have PGXS support in contrib at all.  It's
not documented or tested anywhere, so it might as well not exist.



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

Re: [HACKERS] pg_xlogdump

[Tom Lane]

Andres Freund  writes:
> On 2013-02-23 14:54:51 -0800, Jeff Janes wrote:
>> I don't know if the Makefile needs to be taught not to delete it, or taught
>> how to recreate it once deleted.

> It shouldn't be deleted,

I came to the same conclusion and committed that a few minutes before
you posted.

> I independently wonder whether we should remove the PGXS stub from
> xlogdump, given it relies on a full sourcetree available?

I'd just as soon keep its Makefile looking like all the others.

regards, tom lane


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

Re: [HACKERS] pg_xlogdump

[Andres Freund]

On 2013-02-23 14:54:51 -0800, Jeff Janes wrote:
> If I run "make clean", or "make maintainer-clean", this deletes the file
> contrib/pg_xlogdump/rmgrdesc.c.   And then config/make doesn't know how to
> get it back again.
> 
> I don't know if the Makefile needs to be taught not to delete it, or taught
> how to recreate it once deleted.

It shouldn't be deleted, I think neither Alvaro nor me did notice this
because it only matters in non-vpath builds...

Patch attached.

I independently wonder whether we should remove the PGXS stub from
xlogdump, given it relies on a full sourcetree available?

Andres

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/contrib/pg_xlogdump/Makefile b/contrib/pg_xlogdump/Makefile
index f381967..b454c02 100644
--- a/contrib/pg_xlogdump/Makefile
+++ b/contrib/pg_xlogdump/Makefile
@@ -1,7 +1,7 @@
 # contrib/pg_xlogdump/Makefile
 
 PGFILEDESC = "pg_xlogdump"
-PGAPPICON=win32
+PGAPPICON = win32
 
 PROGRAM = pg_xlogdump
 OBJS = pg_xlogdump.o compat.o xlogreader.o rmgrdesc.o \
@@ -10,7 +10,7 @@ OBJS = pg_xlogdump.o compat.o xlogreader.o rmgrdesc.o \
 RMGRDESCSOURCES = $(notdir $(wildcard $(top_srcdir)/src/backend/access/rmgrdesc/*desc.c))
 RMGRDESCOBJS = $(patsubst %.c,%.o,$(RMGRDESCSOURCES))
 
-EXTRA_CLEAN = $(RMGRDESCSOURCES) xlogreader.c rmgrdesc.c
+EXTRA_CLEAN = $(RMGRDESCSOURCES) xlogreader.c
 
 ifdef USE_PGXS
 PG_CONFIG = pg_config
@@ -25,7 +25,7 @@ endif
 
 override CPPFLAGS := -DFRONTEND $(CPPFLAGS)
 
-rmgrdesc.c xlogreader.c: % : $(top_srcdir)/src/backend/access/transam/%
+xlogreader.c: % : $(top_srcdir)/src/backend/access/transam/%
 	rm -f $@ && $(LN_S) $< .
 
 $(RMGRDESCSOURCES): % : $(top_srcdir)/src/backend/access/rmgrdesc/%

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

Re: [HACKERS] pg_xlogdump

[Jeff Janes]

On Fri, Feb 22, 2013 at 11:58 AM, Alvaro Herrera
wrote:

> Andres Freund wrote:
> > On 2013-02-13 12:09:37 -0300, Alvaro Herrera wrote:
> > > Here's an updated version of pg_xlogdump.  This is rebased on top of
> the
> > > committed xlogreader, palloc restructuring and libpgcommon, PG_RMGR
> > > stuff, and is basically a revamped version of what Andres submitted in
> > >
> http://www.postgresql.org/message-id/1357672187-7693-5-git-send-email-and...@2ndquadrant.com
> >
> > Two tiny followup bits, I had fixed since:
> > * one copy-and-paste-o in an error message
> > * replace stupid directory verification implementation
> > * fix include in compat.c to include utils/timestamp.h instead of
> >   datatype/
>
> Applied with some additional fixes.
>

Hi Álvaro,

If I run "make clean", or "make maintainer-clean", this deletes the file
contrib/pg_xlogdump/rmgrdesc.c.   And then config/make doesn't know how to
get it back again.

I don't know if the Makefile needs to be taught not to delete it, or taught
how to recreate it once deleted.

Thanks,

Jeff

Re: [HACKERS] pg_xlogdump

[Andres Freund]

On 2013-02-22 16:58:37 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > On 2013-02-13 12:09:37 -0300, Alvaro Herrera wrote:
> > > Here's an updated version of pg_xlogdump.  This is rebased on top of the
> > > committed xlogreader, palloc restructuring and libpgcommon, PG_RMGR
> > > stuff, and is basically a revamped version of what Andres submitted in
> > > http://www.postgresql.org/message-id/1357672187-7693-5-git-send-email-and...@2ndquadrant.com
> > 
> > Two tiny followup bits, I had fixed since:
> > * one copy-and-paste-o in an error message
> > * replace stupid directory verification implementation
> > * fix include in compat.c to include utils/timestamp.h instead of
> >   datatype/
> 
> Applied with some additional fixes.

Thanks!

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump

[Alvaro Herrera]

Andres Freund wrote:
> On 2013-02-13 12:09:37 -0300, Alvaro Herrera wrote:
> > Here's an updated version of pg_xlogdump.  This is rebased on top of the
> > committed xlogreader, palloc restructuring and libpgcommon, PG_RMGR
> > stuff, and is basically a revamped version of what Andres submitted in
> > http://www.postgresql.org/message-id/1357672187-7693-5-git-send-email-and...@2ndquadrant.com
> 
> Two tiny followup bits, I had fixed since:
> * one copy-and-paste-o in an error message
> * replace stupid directory verification implementation
> * fix include in compat.c to include utils/timestamp.h instead of
>   datatype/

Applied with some additional fixes.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] pg_xlogdump

[Andres Freund]

On 2013-02-13 12:09:37 -0300, Alvaro Herrera wrote:
> Here's an updated version of pg_xlogdump.  This is rebased on top of the
> committed xlogreader, palloc restructuring and libpgcommon, PG_RMGR
> stuff, and is basically a revamped version of what Andres submitted in
> http://www.postgresql.org/message-id/1357672187-7693-5-git-send-email-and...@2ndquadrant.com

Two tiny followup bits, I had fixed since:
* one copy-and-paste-o in an error message
* replace stupid directory verification implementation
* fix include in compat.c to include utils/timestamp.h instead of
  datatype/

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/contrib/pg_xlogdump/compat.c b/contrib/pg_xlogdump/compat.c
index a3c98c6..dc54bad 100644
--- a/contrib/pg_xlogdump/compat.c
+++ b/contrib/pg_xlogdump/compat.c
@@ -21,7 +21,7 @@
 #include "catalog/catalog.h"
 #include "catalog/pg_tablespace.h"
 #include "common/fe_memutils.h"
-#include "datatype/timestamp.h"
+#include "utils/timestamp.h"
 #include "lib/stringinfo.h"
 #include "storage/relfilenode.h"
 
diff --git a/contrib/pg_xlogdump/pg_xlogdump.c b/contrib/pg_xlogdump/pg_xlogdump.c
index c2ed1b8..9d8597f 100644
--- a/contrib/pg_xlogdump/pg_xlogdump.c
+++ b/contrib/pg_xlogdump/pg_xlogdump.c
@@ -12,6 +12,7 @@
 #define FRONTEND 1
 #include "postgres.h"
 
+#include 
 #include 
 
 #include "rmgrdesc.h"
@@ -64,19 +65,16 @@ fatal_error(const char *fmt,...)
 }
 
 /*
- * Check whether directory exists and whether we can open it.
- * errno is kept set so that the caller can report errors.
+ * Check whether directory exists and whether we can open it. Keep errno set so
+ * that the caller can report errors somewhat more accurate.
  */
 static bool
 verify_directory(const char *directory)
 {
-	int			fd = open(directory, O_DIRECTORY | O_RDONLY);
-
-	if (fd < 0)
+	DIR *dir = opendir(directory);
+	if (dir == NULL)
 		return false;
-
-	close(fd);
-	errno = 0;	/* ignore errors in this case */
+	closedir(dir);
 	return true;
 }
 
@@ -560,7 +558,7 @@ main(int argc, char **argv)
 		else if (!XLByteInSeg(private.startptr, segno))
 		{
 			fprintf(stderr,
-	"%s: end log position %X/%X is not inside file \"%s\"\n",
+	"%s: start log position %X/%X is not inside file \"%s\"\n",
 	progname,
 	(uint32) (private.startptr >> 32),
 	(uint32) private.startptr,

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

[HACKERS] pg_xlogdump

[Alvaro Herrera]

Here's an updated version of pg_xlogdump.  This is rebased on top of the
committed xlogreader, palloc restructuring and libpgcommon, PG_RMGR
stuff, and is basically a revamped version of what Andres submitted in
http://www.postgresql.org/message-id/1357672187-7693-5-git-send-email-and...@2ndquadrant.com

I also attach a patch to move the relpathbackend() function to
src/common.  On top of that, it's trivial to change pg_xlogdump and
remove the "uninplemented" stub we're currently using.  (I also tried it
with a real implementation of relpath() that was mostly a duplicate of
the backend's relpath(), but I didn't like the duplication at all even
though I stripped the unnecessary bits).

(The more adventorous might think about moving timestamp_to_str to
src/common, as well, but this isn't a simple task: it depends on some
backend global state variables such as GUC vars, so it requires detailed
surgery.  I think it's a worthy goal nonetheless, because it'd allow us
to reduce useless duplication in ECPG, but it's not a 9.3 project)

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
*** /dev/null
--- b/contrib/pg_xlogdump/Makefile
***
*** 0 
--- 1,32 
+ # contrib/pg_xlogdump/Makefile
+ 
+ PGFILEDESC = "pg_xlogdump"
+ PGAPPICON=win32
+ 
+ PROGRAM = pg_xlogdump
+ OBJS = pg_xlogdump.o compat.o xlogreader.o rmgrdesc.o \
+ 	$(RMGRDESCOBJS) $(WIN32RES)
+ 
+ RMGRDESCSOURCES = $(notdir $(wildcard $(top_srcdir)/src/backend/access/rmgrdesc/*desc.c))
+ RMGRDESCOBJS = $(patsubst %.c,%.o,$(RMGRDESCSOURCES))
+ 
+ EXTRA_CLEAN = $(RMGRDESCSOURCES) xlogreader.c rmgrdesc.c
+ 
+ ifdef USE_PGXS
+ PG_CONFIG = pg_config
+ PGXS := $(shell $(PG_CONFIG) --pgxs)
+ include $(PGXS)
+ else
+ subdir = contrib/pg_xlogdump
+ top_builddir = ../..
+ include $(top_builddir)/src/Makefile.global
+ include $(top_srcdir)/contrib/contrib-global.mk
+ endif
+ 
+ override CPPFLAGS := -DFRONTEND $(CPPFLAGS)
+ 
+ rmgrdesc.c xlogreader.c: % : $(top_srcdir)/src/backend/access/transam/%
+ 	rm -f $@ && $(LN_S) $< .
+ 
+ $(RMGRDESCSOURCES): % : $(top_srcdir)/src/backend/access/rmgrdesc/%
+ 	rm -f $@ && $(LN_S) $< .
*** /dev/null
--- b/contrib/pg_xlogdump/compat.c
***
*** 0 
--- 1,83 
+ /*-
+  *
+  * compat.c
+  *		Reimplementations of various backend functions.
+  *
+  * Portions Copyright (c) 2012, PostgreSQL Global Development Group
+  *
+  * IDENTIFICATION
+  *		contrib/pg_xlogdump/compat.c
+  *
+  * This file contains client-side implementations for various backend
+  * functions that the rm_desc functions in *desc.c files rely on.
+  *
+  *-
+  */
+ 
+ /* ugly hack, same as in e.g pg_controldata */
+ #define FRONTEND 1
+ #include "postgres.h"
+ 
+ #include "catalog/catalog.h"
+ #include "catalog/pg_tablespace.h"
+ #include "common/fe_memutils.h"
+ #include "datatype/timestamp.h"
+ #include "lib/stringinfo.h"
+ #include "storage/relfilenode.h"
+ 
+ const char *
+ timestamptz_to_str(TimestampTz dt)
+ {
+ 	return "unimplemented-timestamp";
+ }
+ 
+ /*
+  * Table of fork names.
+  *
+  * needs to be synced with src/backend/catalog/catalog.c
+  */
+ const char *forkNames[] = {
+ 	"main",		/* MAIN_FORKNUM */
+ 	"fsm",		/* FSM_FORKNUM */
+ 	"vm",		/* VISIBILITYMAP_FORKNUM */
+ 	"init"		/* INIT_FORKNUM */
+ };
+ #define FORKNAMECHARS	4
+ 
+ /*
+  * relpathbackend
+  *
+  * A pg_xlogdump implementation of the backend function of the same name.
+  * This one doesn't accept non-permanent relfilenodes.
+  */
+ char *
+ relpathbackend(RelFileNode rnode, BackendId backend, ForkNumber forknum)
+ {
+ 	return pg_strdup("unimplemented-relpath");
+ }
+ 
+ /*
+  * Provide a hacked up compat layer for StringInfos so xlog desc functions can
+  * be linked/called.
+  */
+ void
+ appendStringInfo(StringInfo str, const char *fmt, ...)
+ {
+ 	va_list		args;
+ 
+ 	va_start(args, fmt);
+ 	vprintf(fmt, args);
+ 	va_end(args);
+ }
+ 
+ void
+ appendStringInfoString(StringInfo str, const char *string)
+ {
+ 	appendStringInfo(str, "%s", string);
+ }
+ 
+ void
+ appendStringInfoChar(StringInfo str, char ch)
+ {
+ 	appendStringInfo(str, "%c", ch);
+ }
*** /dev/null
--- b/contrib/pg_xlogdump/pg_xlogdump.c
***
*** 0 
--- 1,674 
+ /*-
+  *
+  * pg_xlogdump.c - decode and display WAL
+  *
+  * Copyright (c) 2012, PostgreSQL Global Development Group
+  *
+  * IDENTIFICATION
+  *		  contrib/pg_xlogdump/pg_xlogdump.c
+  *-
+  */
+ 
+ #define FRONTEND 1
+ #include "postgres.h"
+ 
+ #include 
+ 
+ #include "rmgrdesc.h"
+ 
+ #include "access/xlog.h"
+ #include "access/xlogreader.h"
+ #include "access/transam.h"
+ #include "catalog/catalog.h"
+ #include "common/fe_memutils.h