Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2014-01-02 Thread Robert Haas
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

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2014-01-02 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: On Tue, Dec 31, 2013 at 12:20 PM, Tom Lane t...@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

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2014-01-02 Thread 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 -

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2014-01-02 Thread Mark Dilger
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

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2014-01-02 Thread Andres Freund
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 

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2014-01-02 Thread Tom Lane
Mark Dilger markdil...@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

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2014-01-02 Thread Mark Dilger
I completely understand the padding issues that you are dealing with.  I was mostly curious why Tom wanted to use asserts to double-check the code in one place, while happily not doing so in what seemed the same kind of situation elsewhere. He has since made the reason for that clear. On

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2014-01-02 Thread Mark Dilger
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   

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2014-01-02 Thread Tom Lane
Mark Dilger markdil...@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; }

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2013-12-31 Thread Tom Lane
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

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2013-12-31 Thread Mark Dilger
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

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2013-12-31 Thread Tom Lane
Mark Dilger markdil...@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

Re: [HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2013-12-31 Thread Mark Dilger
A quick grep through the code reveals lots of examples, so I'll just paste the first ones I notice.  There are references to sizeof(Oid), sizeof(uint32), sizeof(bool), and sizeof(uint8) that clearly refer to fields in structs that the macros refer to implicitly, but there is no way for the

[HACKERS] fix_PGSTAT_NUM_TABENTRIES_macro patch

2013-12-30 Thread Mark Dilger
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