Обсуждение: Use of SizeOfIptrData - is that obsolete?
Hi,
--
I happened to notice this comment in src/include/storage/itemptr.h
* Note: because there is an item pointer in each tuple header and index
* tuple header on disk, it's very important not to waste space with
* structure padding bytes. The struct is designed to be six bytes long
* (it contains three int16 fields) but a few compilers will pad it to
* eight bytes unless coerced. We apply appropriate persuasion where
* possible, and to cope with unpersuadable compilers, we try to use
* "SizeOfIptrData" rather than "sizeof(ItemPointerData)" when computing
* on-disk sizes.
*/
Is that now obsolete? I mean I can still find one reference in src/backend/executor/nodeTidscan.c, which uses SizeOfIptrData instead of sizeof (ItemPointerData), but all other places such as heapam_xlog.h now use sizeof operator. It was changed in 2c03216d831160b when we moved a bunch of xlog related stuff from htup.h to this new file. Hard to tell if there were other users before that and they were all dropped in this one commit or various commits leading to this.
Thanks,
Pavan
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
PostgreSQL Development, 24x7 Support, Training & Services
Pavan Deolasee <pavan.deolasee@gmail.com> writes:
> I happened to notice this comment in src/include/storage/itemptr.h
> * Note: because there is an item pointer in each tuple header and index
> * tuple header on disk, it's very important not to waste space with
> * structure padding bytes. The struct is designed to be six bytes long
> * (it contains three int16 fields) but a few compilers will pad it to
> * eight bytes unless coerced. We apply appropriate persuasion where
> * possible, and to cope with unpersuadable compilers, we try to use
> * "SizeOfIptrData" rather than "sizeof(ItemPointerData)" when computing
> * on-disk sizes.
> */
> Is that now obsolete?
Realistically, because struct HeapTupleHeaderData contains a field of
type ItemPointerData, it's probably silly to imagine that we can save
anything if the compiler can't be persuaded to believe that
sizeof(ItemPointerData) is 6. It may well be that the structure pragmas
work on everything that wouldn't natively believe that anyway.
regards, tom lane
On Tue, Sep 20, 2016 at 8:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Pavan Deolasee <pavan.deolasee@gmail.com> writes:
> I happened to notice this comment in src/include/storage/itemptr.h
> * Note: because there is an item pointer in each tuple header and index
> * tuple header on disk, it's very important not to waste space with
> * structure padding bytes. The struct is designed to be six bytes long
> * (it contains three int16 fields) but a few compilers will pad it to
> * eight bytes unless coerced. We apply appropriate persuasion where
> * possible, and to cope with unpersuadable compilers, we try to use
> * "SizeOfIptrData" rather than "sizeof(ItemPointerData)" when computing
> * on-disk sizes.
> */
> Is that now obsolete?
Realistically, because struct HeapTupleHeaderData contains a field of
type ItemPointerData, it's probably silly to imagine that we can save
anything if the compiler can't be persuaded to believe that
sizeof(ItemPointerData) is 6. It may well be that the structure pragmas
work on everything that wouldn't natively believe that anyway.
Yeah, that's what I thought and rest of the code seems to make that assumption as well. Attached patch removes the last remaining reference to SizeOfIptrData and also removes the macro and the associated comment. TBH I couldn't fully trace how the TID array gets generated in nodeTidscan.c, but I'm fairly confident that this should not break anything because this was the only remaining reference.
While htup.h refactoring happened in 9.5, I don't see any point in back patching this.
Thanks,
Pavan
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
PostgreSQL Development, 24x7 Support, Training & Services
Вложения
Pavan Deolasee <pavan.deolasee@gmail.com> writes:
> On Tue, Sep 20, 2016 at 8:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Realistically, because struct HeapTupleHeaderData contains a field of
>> type ItemPointerData, it's probably silly to imagine that we can save
>> anything if the compiler can't be persuaded to believe that
>> sizeof(ItemPointerData) is 6. It may well be that the structure pragmas
>> work on everything that wouldn't natively believe that anyway.
> Yeah, that's what I thought and rest of the code seems to make that
> assumption as well. Attached patch removes the last remaining reference to
> SizeOfIptrData and also removes the macro and the associated comment.
I thought removing the comment altogether was not appropriate, because
it remains true that you want to work really hard to ensure that
sizeof(ItemPointerData) is 6. We're just giving up on pretense of support
for compilers that don't believe that. I'm half tempted to introduce a
StaticAssert about it, but refrained for the moment.
> While htup.h refactoring happened in 9.5, I don't see any point in back
> patching this.
Agreed. Pushed to HEAD only.
regards, tom lane
On Fri, Sep 23, 2016 at 12:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I thought removing the comment altogether was not appropriate, because
it remains true that you want to work really hard to ensure that
sizeof(ItemPointerData) is 6. We're just giving up on pretense of support
for compilers that don't believe that
Makes sense.
. I'm half tempted to introduce a
StaticAssert about it, but refrained for the moment.
I also thought about that and it probably makes sense, at least to see how buildfarm behaves. One reason to do so is that I did not find any discussion or evidence of why SizeOfIptrData magic is no longer necessary and to see if it was an unintentional change at some point.
> While htup.h refactoring happened in 9.5, I don't see any point in back
> patching this.
Agreed. Pushed to HEAD only.
Thanks.
Thanks,
Pavan
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
PostgreSQL Development, 24x7 Support, Training & Services