Re: fix_PGSTAT_NUM_TABENTRIES_macro patch

On Tue, Dec 31, 2013 at 12:20 PM, 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.

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