pg_xlogdump --stats

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@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

At 2014-06-10 18:04:13 +0900, furuyao@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

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

At 2014-06-30 05:19:10 +0000, dilip.kumar@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

At 2014-06-30 12:05:06 +0530, ams@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

On Wed, Jun 4, 2014 at 1:47 PM, Abhijit Menon-Sen <ams@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

At 2014-07-01 16:39:57 +0300, marti@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

On Tue, Jul 1, 2014 at 11:13 PM, Abhijit Menon-Sen <ams@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

At 2014-07-02 04:20:31 +0300, marti@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

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

At 2014-06-30 05:19:10 +0000, dilip.kumar@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

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

-----Original Message-----
From: Abhijit Menon-Sen [mailto:ams@2ndQuadrant.com]
Sent: 04 July 2014 12:07
To: Dilip kumar
Cc: pgsql-hackers@postgresql.org; furuyao@pm.nttdata.co.jp
Subject: Re: [HACKERS] pg_xlogdump --stats

At 2014-06-30 05:19:10 +0000, dilip.kumar@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

At 2014-07-04 08:38:17 +0000, dilip.kumar@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

On 2014-07-04 14:16:42 +0530, Abhijit Menon-Sen wrote:

At 2014-07-04 08:38:17 +0000, dilip.kumar@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

At 2014-07-04 10:54:08 +0200, andres@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

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.

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
@@ -27,8 +27,8 @@
#include "storage/standby.h"
#include "utils/relmapper.h"

-#define PG_RMGR(symname,name,redo,desc,startup,cleanup) \
-	{ name, desc, },
+#define PG_RMGR(symname,name,redo,desc,identify,startup,cleanup) \
+	{ name, desc, identify, },

const RmgrDescData RmgrDescTable[RM_MAX_ID + 1] = {
#include "access/rmgrlist.h"
diff --git a/contrib/pg_xlogdump/rmgrdesc.h b/contrib/pg_xlogdump/rmgrdesc.h
index d964118..da805c5 100644
--- a/contrib/pg_xlogdump/rmgrdesc.h
+++ b/contrib/pg_xlogdump/rmgrdesc.h
@@ -14,6 +14,7 @@ typedef struct RmgrDescData
{
const char *rm_name;
void		(*rm_desc) (StringInfo buf, XLogRecord *record);
+	const char *(*rm_identify) (uint8 info);
} RmgrDescData;

Looks like that should at least partially have been in the other patch?
The old PG_RMGR looks like it wouldn't compile with the rm_identity
argument added?

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

At 2014-07-04 11:34:21 +0200, andres@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

On 2014-07-04 15:34:14 +0530, Abhijit Menon-Sen wrote:

At 2014-07-04 11:34:21 +0200, andres@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

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