Re: fix_PGSTAT_NUM_TABENTRIES_macro patch

On Tue, Dec 31, 2013 at 12:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Mark Dilger <markdilger@yahoo.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.

Ick.

>> 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.

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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company