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


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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to