Re: pg_xlogdump --stats

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