Re: fix_PGSTAT_NUM_TABENTRIES_macro patch

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Dec 31, 2013 at 12:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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:
>> ...

> Rather than using a union, I'd be inclined to declare one type that's
> just the header (PgStat_MsgTabstat_Hdr), and then work out
> PGSTAT_NUM_TABENTRIES based on sizeof(PgStat_MsgTabstat_Hdr), and then
> declare PgStat_MsgTabstat as a two-element struct, the header struct
> followed by an array of entries. That is indeed a bit of notational
> churn but it seems worth it to me.

That approach would fail to account for any alignment padding occurring
just before the m_entry array, though. That could happen if the array
members contain some 8-byte fields while there are none in the header
part. I think it would net out to the same amount of notational change
in the code proper as my approach, since either way you end up with a
nested struct containing the header fields.

It occurs to me that, rather than trying to improve the struct definition
methodology, maybe we should just add static asserts to catch any
inconsistency here. It wouldn't be that hard:

#define PGSTAT_MAX_MSG_SIZE 1000
#define PGSTAT_MSG_PAYLOAD (PGSTAT_MAX_MSG_SIZE - sizeof(PgStat_MsgHdr))
... all else in pgstat.h as before ...

StaticAssertStmt(sizeof(PgStat_MsgTabstat) <= PGSTAT_MAX_MSG_SIZE,
'bad PgStat_MsgTabstat size');
... and similarly for other pgstat message structs ...

This would possibly lead to machine-dependent results if alignment comes
into play, but it's reasonable to suppose that we'd find out about any
mistakes as soon as they hit the buildfarm. So this might be an
acceptable tradeoff for not having to change source code.

regards, tom lane