Re: fix_PGSTAT_NUM_TABENTRIES_macro patch

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