Re: [HACKERS] Use of SizeOfIptrData - is that obsolete?

[Pavan Deolasee]

On Fri, Sep 23, 2016 at 12:04 AM, Tom Lane  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

Re: [HACKERS] Use of SizeOfIptrData - is that obsolete?

[Tom Lane]

Pavan Deolasee  writes:
> On Tue, Sep 20, 2016 at 8:34 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Use of SizeOfIptrData - is that obsolete?

[Pavan Deolasee]

On Tue, Sep 20, 2016 at 8:34 PM, Tom Lane  wrote:

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


pg_remove_itemptr_size.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Use of SizeOfIptrData - is that obsolete?

[Tom Lane]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Use of SizeOfIptrData - is that obsolete?

[Pavan Deolasee]

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