Re: pg_xlogdump --stats - Mailing list pgsql-hackers

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