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 a...@2ndquadrant.com
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 dirent.h
 #include unistd.h
 
-#include access/xlog.h
 #include access/xlogreader.h
 #include access/transam.h

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 about the static buffer used in
append_init(). That means the returned value from heap_identity() is
only valid until the next call. That at the very least needs to 

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

[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

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

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 a...@2ndquadrant.com
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(buf, 

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

[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

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

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

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

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

[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

[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 stdio.h
  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-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 xlocale.h. */
 #undef LOCALE_T_IN_XLOCALE
@@ -741,9 +741,6 @@
 /* Define to 1 if your sys/time.h 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 xlocale.h. */
 /* #undef LOCALE_T_IN_XLOCALE */
@@ -601,10 +601,6 @@
 /* Define to 1 if your sys/time.h 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 result is 

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

[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-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 dirent.h
 #include unistd.h
 
+#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_XACT_ASSIGNMENT:
+	rec = ASSIGNMENT;
+	break;
+case XLOG_XACT_COMMIT_COMPACT:
+

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 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 = REWRITE;
+			break;
+	}
+
+	if (info  XLOG_HEAP_INIT_PAGE)
+		id = 

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,
 +  
 rec_len+fpi_len, 100*(double)(rec_len+fpi_len)/total_len);
 + }
 + }
 + }

Some additional leaking here.

 

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]

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

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

[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 stdio.h
 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=\$LONG_LONG_UINT_FORMAT\
 

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

[Marti Raudsepp]

On Wed, Jun 4, 2014 at 1:47 PM, Abhijit Menon-Sen a...@2ndquadrant.com 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-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 Tue, Jul 1, 2014 at 11:13 PM, Abhijit Menon-Sen a...@2ndquadrant.com 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-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

[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-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 dirent.h
 #include unistd.h
 
+#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_RESTORE_POINT:
+	rec = RESTORE_POINT;
+	break;
+case XLOG_FPW_CHANGE:
+	

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 a...@2ndquadrant.com
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
 
  varlistentry
   termoption-z/option/term
-  termoption--stats[=record]/option/term
+  termoption--stats/option/term
   listitem
para
 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.
+   /para
+  /listitem
+ /varlistentry
+
+ varlistentry
+  termoption-Z/option/term
+  termoption--record-stats/option/term
+  listitem
+   para
+Display summary statistics (number and size of records and
+full-page images) instead of individual records.
/para
   /listitem
  /varlistentry

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

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