Re: fix_PGSTAT_NUM_TABENTRIES_macro patch - Mailing list pgsql-hackers

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