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
partof the  FPI. And this means that the record length will be > 0 even if all  the record data has been removed due to
theFPI.
 

>  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