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.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers