Re: pg_xlogdump --stats

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, --stats show 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 be
written down explicitly somewhere.

> diff --git a/src/include/c.h b/src/include/c.h
> index 2ceaaf6..cf3cbd1 100644
> --- a/src/include/c.h
> +++ b/src/include/c.h
> @@ -867,6 +867,9 @@ typedef NameData *Name;
> * ----------------------------------------------------------------
> */
>
> +#define INT64_FORMAT "%" INT64_MODIFIER "d"
> +#define UINT64_FORMAT "%" INT64_MODIFIER "u"
> +

That's already in there afaics:

/* snprintf format strings to use for 64-bit integers */
#define INT64_FORMAT "%" INT64_MODIFIER "d"
#define UINT64_FORMAT "%" INT64_MODIFIER "u"

> +/*
> + * Returns a string describing an XLogRecord, consisting of its identity
> + * optionally followed by a colon, a space, and a further description.
> + */
> +static void
> +xlog_outdesc(StringInfo buf, RmgrId rmid, XLogRecord *record)
> +{
> + const char *id;
> +
> + id = RmgrTable[rmid].rm_identify(record->xl_info);
> + if (id == NULL)
> + appendStringInfo(buf, "%X", record->xl_info);

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

Greetings,

Andres Freund

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