Thread: fix_PGSTAT_NUM_TABENTRIES_macro patch

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.

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

> As an aside, the PGSTAT_MSG_PAYLOAD define from which
> these field sizes are being subtracted is a bit of a WAG, if
> I am reading the code correctly, in which case whether the
> two additional fields are subtracted from that WAG is perhaps
> not critical.� But the code is certainly easier to understand if
> the macro matches the definition of the struct.

Yeah, it seems unlikely that exceeding the 1000-byte target, even by quite
a lot, would cause any performance problem on most platforms --- a quick
check says that the MTU on the loopback interface is near 16K on the
machines I have handy.  So that dampens my enthusiasm for making invasive
code changes to ensure we meet the target exactly.  Still, the next
oversight of this sort might introduce a larger error.

Does anyone have any other ideas about making this coding more robust?
        regards, tom lane

Mark Dilger <markdilger@yahoo.com> writes:
> I don't care for the places in the code that say things like

> ��� foo - sizeof(int)

> where "int" refers implicitly to a specific variable or struct field, but
> you have to remember that and change it by hand if you change the
> type of the variable or struct.

> But this sort of code is quite common in postgres, and not
> at all unique to src/include/pgstat.h.

Really?  I think we're using offsetof for this type of thing in most
places.
        regards, tom lane

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

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

I wrote:
> 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 ...

After further thought it seems to me that this is a desirable approach,
because it is a direct check of the property we want, and will complain
about *any* mistake that results in too-large struct sizes.  The other
ideas that were kicked around upthread still left a lot of ways to mess up
the array size calculations, for instance referencing the wrong array
element type in the sizeof calculation.  So unless anyone has a concrete
objection, I'll go put in something like this along with Mark's fix.
        regards, tom lane

<div style="color:#000; background-color:#fff; font-family:HelveticaNeue, Helvetica Neue, Helvetica, Arial, Lucida
Grande,sans-serif;font-size:12pt">I still don't understand why this case in src/include/pgstat.h<br />is different from
caseselsewhere in the code.  Taken from<br />src/include/access/heapam_xlog.h:<br /><br /><br />typedef struct
xl_heap_header<br/>{<br />    uint16      t_infomask2;<br />    uint16      t_infomask;<br />    uint8       t_hoff;<br
/>}xl_heap_header;<br /><br />#define SizeOfHeapHeader    (offsetof(xl_heap_header, t_hoff) + sizeof(uint8))<br /><br
/><br/><br />Now, if somebody changed t_hoff to be a uint16, that SizeOfHeapHeader<br />macro would be wrong.  Should
weput a static assert in the code for that?<br />I have no objection, but it seems you like the static assert in one
place<br/>and not the other, and (perhaps due to some incredible ignorance on my<br />part) I cannot see why.  I tried
lookingfor an assert of this kind already in<br />the code.  The use of this macro is in
src/backend/access/heap/heapam.c,<br/>but I didn't see any asserts for it, though there are lots of asserts for
other<br/>stuff.  Maybe I just didn't recognize it?<br /><br /><br />mark<br /><div class="yahoo_quoted"
style="display:block;"><br /><br /><div style="font-family: HelveticaNeue, Helvetica Neue, Helvetica, Arial, Lucida
Grande,sans-serif; font-size: 12pt;"><div style="font-family: HelveticaNeue, Helvetica Neue, Helvetica, Arial, Lucida
Grande,sans-serif; font-size: 12pt;"><div dir="ltr"><font face="Arial" size="2"> On Thursday, January 2, 2014 2:05 PM,
TomLane <tgl@sss.pgh.pa.us> wrote:<br /></font></div><div class="y_msg_container">I wrote:<br clear="none" />>
Itoccurs to me that, rather than trying to improve the struct definition<br clear="none" />> methodology, maybe we
shouldjust add static asserts to catch any<br clear="none" />> inconsistency here.  It wouldn't be that hard:<br
clear="none"/><br clear="none" />> #define PGSTAT_MAX_MSG_SIZE    1000<br clear="none" />> #define
PGSTAT_MSG_PAYLOAD   (PGSTAT_MAX_MSG_SIZE - sizeof(PgStat_MsgHdr))<br clear="none" />> ... all else in pgstat.h as
before...<br clear="none" /><br clear="none" />> StaticAssertStmt(sizeof(PgStat_MsgTabstat) <=
PGSTAT_MAX_MSG_SIZE,<brclear="none" />>         'bad PgStat_MsgTabstat size');<br clear="none" />> ... and
similarlyfor other pgstat message structs ...<br clear="none" /><br clear="none" />After further thought it seems to me
thatthis is a desirable approach,<br clear="none" />because it is a direct check of the property we want, and will
complain<brclear="none" />about *any* mistake that results in too-large struct sizes.  The other<br clear="none"
/>ideasthat were kicked around upthread still left a lot of ways to mess up<br clear="none" />the array size
calculations,for instance referencing the wrong array<br clear="none" />element type in the sizeof calculation.  So
unlessanyone has a concrete<br clear="none" />objection, I'll go put in something like this along with Mark's fix.<div
class="yqt0876931955"id="yqtfd25444"><br clear="none" /><br clear="none" />            regards, tom lane<br
clear="none"/><br clear="none" /><br clear="none" />-- <br clear="none" />Sent via pgsql-hackers mailing list (<a
href="mailto:pgsql-hackers@postgresql.org"shape="rect"
ymailto="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<brclear="none" />To make changes to
yoursubscription:<br clear="none" /><a href="http://www.postgresql.org/mailpref/pgsql-hackers" shape="rect"
target="_blank">http://www.postgresql.org/mailpref/pgsql-hackers</a><brclear="none" /></div><br /><br
/></div></div></div></div></div>

On 2014-01-02 15:15:58 -0800, Mark Dilger wrote:
> I still don't understand why this case in src/include/pgstat.h
> is different from cases elsewhere in the code.  Taken from
> src/include/access/heapam_xlog.h:
>
>
> typedef struct xl_heap_header
> {
>     uint16      t_infomask2;
>     uint16      t_infomask;
>     uint8       t_hoff;
> } xl_heap_header;
>
> #define SizeOfHeapHeader    (offsetof(xl_heap_header, t_hoff) + sizeof(uint8))
>
>
>
> Now, if somebody changed t_hoff to be a uint16, that SizeOfHeapHeader
> macro would be wrong.  Should we put a static assert in the code for that?

The reason the various SizeOfHeapHeader are written that way is that we
do not want to uselessly store trailing padding in the
WAL. E.g. sizeof(xl_heap_header) will be 6bytes, but SizeOfHeapHeader
will be 5.
I don't see an easy way to guarantee this with asserts and I think you'd
notice pretty fast if you got things wrong there because WAL replay will
just have incomplete data.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Mark Dilger <markdilger@yahoo.com> writes:
> I still don't understand why this case in src/include/pgstat.h
> is different from cases elsewhere in the code.

The reason why I'm exercised about it is that (a) somebody actually made a
mistake of this type, and (b) it wasn't caught by any automated testing.

The catalog and WAL-related examples you cite would probably crash
and burn in fairly obvious ways if somebody broke them --- for instance,
the most likely way to break SizeOfHeapHeader would be by adding another
field after t_hoff, but we'd notice that before long because said field
would be corrupted on arrival at a replication slave.

In contrast, messing up the pgstats message sizes would have no
consequences worse than a hard-to-detect, and probably platform-specific,
performance penalty for stats transmission.  So unless we think that's
of absolutely zero concern, adding a mechanism to make such bugs more
apparent seems useful.

I'm not against adding more assertions elsewhere, but it's a bit hard to
see what those asserts should test.  I don't see any practical way to
assert that field X is the last one in its struct, for instance.
        regards, tom lane

Mark Dilger <markdilger@yahoo.com> writes:
> The mechanism that occurs to me (and I'm not wedded to
> this idea) is:

> typedef uint8 T_HOFF_TYPE;
> typedef struct xl_heap_header
> {
> ������� uint16��������� t_infomask2;
> ������� uint16��������� t_infomask;
> ������� T_HOFF_TYPE������������ t_hoff;
> } xl_heap_header;

> #define SizeOfHeapHeader������� (offsetof(xl_heap_header, t_hoff) + sizeof(T_HOFF_TYPE))

Meh.  That does nothing for the "add a field in the wrong place" risk.
Yes, it would prevent people from changing the type of t_hoff without
updating the macro --- but I'm not convinced that defending against that
alone is worth any notational pain.  If you're changing t_hoff's type
without looking fairly closely at every reference to t_hoff, you're
practicing unsafe programming to begin with.

I wonder though if we could invent a macro that produces the sizeof
a struct field, and then use that in macros like this.  Something like

#define field_sizeof(typename, fieldname) \sizeof(((typename *) NULL)->fieldname)

Compare the default definition of offsetof ...
        regards, tom lane