Re: fix_PGSTAT_NUM_TABENTRIES_macro patch

I don't care for the places in the code that say things like

    foo - sizeof(int)

where "int" refers implicitly to a specific variable or struct field, but
you have to remember that and change it by hand if you change the
type of the variable or struct.

But this sort of code is quite common in postgres, and not
at all unique to src/include/pgstat.h.  I did not try to tackle
that project-wide, as it would turn a one-line patch (which has
a good chance of being accepted) into a huge patch that
would likely be rejected.

On the other hand, if the community feels this kind of code
cleanup is needed, I might jump in.....

Mark Dilger

On Tuesday, December 31, 2013 9:21 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

Mark Dilger <markdilger(at)yahoo(dot)com> writes:
> In src/include/pgstat.h, the PGSTAT_NUM_TABENTRIES macro
> attempts to subtract off the size of the PgStat_MsgTabstat
> struct up to the m_entry[] field.  This macro was correct up
> until the fields m_block_read_time and m_block_write_time
> were added to that struct, as the macro was not changed to
> include their size.

Yeah, that's a bug.

> This patch fixes the macro.

I'm inclined to think that we should look for a less breakable way to
manage the message size limit; if Robert missed this issue in that patch,
other people are going to miss it in future patches.  The existing coding
isn't really right anyway when you consider possible alignment padding.
(I think the present struct definitions probably don't involve any
padding, but that's not a very future-proof assumption either.)  It'd be
better to somehow drive the calculation from offsetof(m_entry).  I'm not
seeing any way to do that that doesn't require some notational changes
in the code, though.  One way would be to rejigger it like this:

#define PGSTAT_MAX_MSG_SIZE 1000

typedef union PgStat_MsgTabstat
{
    struct {
        PgStat_MsgHdr hdr;
        Oid          databaseid;
        int          nentries;
        int          xact_commit;
        int          xact_rollback;
        PgStat_Counter block_read_time;    /* times in microseconds */
        PgStat_Counter block_write_time;
        PgStat_TableEntry entry[FLEXIBLE_ARRAY_MEMBER];
    } m;
    char padding[PGSTAT_MAX_MSG_SIZE];    /* pad sizeof this union to target */
} PgStat_MsgTabstat;

#define PGSTAT_NUM_TABENTRIES ((PGSTAT_MAX_MSG_SIZE - offsetof(PgStat_MsgTabstat, m.entry)) / sizeof(PgStat_TableEntry))

but then we'd have to run around and replace "m_hdr" with "m.hdr" etc
in the code referencing the message types that use this mechanism.
Kind of annoying.

> As an aside, the PGSTAT_MSG_PAYLOAD define from which
> these field sizes are being subtracted is a bit of a WAG, if
> I am reading the code correctly, in which case whether the
> two additional fields are subtracted from that WAG is perhaps
> not critical.  But the code is certainly easier to understand if
> the macro matches the definition of the struct.

Yeah, it seems unlikely that exceeding the 1000-byte target, even by quite
a lot, would cause any performance problem on most platforms --- a quick
check says that the MTU on the loopback interface is near 16K on the
machines I have handy.  So that dampens my enthusiasm for making invasive
code changes to ensure we meet the target exactly.  Still, the next
oversight of this sort might introduce a larger error.

Does anyone have any other ideas about making this coding more robust?

            regards, tom lane