Re: [HACKERS] questionable code in heap_formtuple()

around line 812 in access/common/heaptuple.c:

len = sizeof *tuple - sizeof tuple->t_bits;

This seems questionable for me.

tuple is a pointer to struct HeaptupleData.

typedef struct HeapTupleData
{
unsigned int t_len; /* length of entire tuple */

[snip]

uint8 t_hoff; /* sizeof tuple header */

bits8 t_bits[MinHeapTupleBitmapSize / 8];
/* bit map of domains */

/* MORE DATA FOLLOWS AT END OF STRUCT */
} HeapTupleData;

I think the code tries to calculate the offset from top of the
structure to t_bits. t_bits is the last structure member of
HeapTupleData, and that would give the offset...

No. since the size of the whole structure is aligned to 2-byte, there
is a "padding" byte after t_bits.

I think more acculate way to calculate the offset is:

len = (char *)&tuple->t_bits[0] - (char *)tuple;

I ran a test and found the first one gives len = 36, while second one
gives 35.

I'm not sure how this affects. maybe nothing (len is aligned to 8-byte
boundary later).
--
Tatsuo Ishii
t-ishii(at)sra(dot)co(dot)jp

> around line 812 in access/common/heaptuple.c:
>
> len = sizeof *tuple - sizeof tuple->t_bits;
>
> This seems questionable for me.

This is interesting. They are getting the sizeof tuple->t_bits, not the
offset, so aren't they getting this very wrong?

They are computing the size of the tuple, minus the t_bits field, which
means nothing, no?

>
> tuple is a pointer to struct HeaptupleData.
>
> typedef struct HeapTupleData
> {
> unsigned int t_len; /* length of entire tuple */
>
> [snip]
>
> uint8 t_hoff; /* sizeof tuple header */
>
> bits8 t_bits[MinHeapTupleBitmapSize / 8];
> /* bit map of domains */
>
> /* MORE DATA FOLLOWS AT END OF STRUCT */
> } HeapTupleData;
>
> I think the code tries to calculate the offset from top of the
> structure to t_bits. t_bits is the last structure member of
> HeapTupleData, and that would give the offset...

Does it?

>
> No. since the size of the whole structure is aligned to 2-byte, there
> is a "padding" byte after t_bits.
>
> I think more acculate way to calculate the offset is:
>
> len = (char *)&tuple->t_bits[0] - (char *)tuple;

Yours is much better.

>
> I ran a test and found the first one gives len = 36, while second one
> gives 35.
>
> I'm not sure how this affects. maybe nothing (len is aligned to 8-byte
> boundary later).

Should affect a lot, if I am understanding it properly. This is also
done in heap_addheader() later in the file.

I just ran a little test:

#include <stdio.h>

struct test {
int x;
int y;
} test;

main()
{
printf("%d\n",sizeof(test.y));
return 0;
}

and with sizeof int == 4, the program returns 4, which is not the offset
of y, but the size of y. 6.3.2 has the same code.

I must be misunderstanding this.

--
Bruce Momjian | 830 Blythe Avenue
maillist(at)candle(dot)pha(dot)pa(dot)us | Drexel Hill, Pennsylvania 19026
+ If your life is a hard drive, | (610) 353-9879(w)
+ Christ can be your backup. | (610) 853-3000(h)

> > around line 812 in access/common/heaptuple.c:
> >
> > len = sizeof *tuple - sizeof tuple->t_bits;
> >
> > This seems questionable for me.

> > I think more acculate way to calculate the offset is:
> >
> > len = (char *)&tuple->t_bits[0] - (char *)tuple;

OK, now I am more confused. Doesn't this work:

len = sizeof(HeapTupleData) - offsetof(HeapTupleData.t_bits);

while your solution is finding the size of the area before t_bits?

--
Bruce Momjian | 830 Blythe Avenue
maillist(at)candle(dot)pha(dot)pa(dot)us | Drexel Hill, Pennsylvania 19026
+ If your life is a hard drive, | (610) 353-9879(w)
+ Christ can be your backup. | (610) 853-3000(h)

> > > around line 812 in access/common/heaptuple.c:
> > >
> > > len = sizeof *tuple - sizeof tuple->t_bits;
> > >
> > > This seems questionable for me.
>
>
> > > I think more acculate way to calculate the offset is:
> > >
> > > len = (char *)&tuple->t_bits[0] - (char *)tuple;
>
> OK, now I am more confused. Doesn't this work:
>
> len = sizeof(HeapTupleData) - offsetof(HeapTupleData.t_bits);
>
> while your solution is finding the size of the area before t_bits?

OK, I finally get it. I was thinking HeapTupleData had the tuple data
in the structure, while obviously it does not.

Sometimes there is no HeapTuple to get the size of at the point you need
it, so I have applied a patch to do

len = offsetof(HeapTupleData.t_bits);

which should fix the obvious problem Tatsuo Ishii found. Does that fix
anything, index people?

--
Bruce Momjian | 830 Blythe Avenue
maillist(at)candle(dot)pha(dot)pa(dot)us | Drexel Hill, Pennsylvania 19026
+ If your life is a hard drive, | (610) 353-9879(w)
+ Christ can be your backup. | (610) 853-3000(h)

Oops, I meant:

len = offsetof(HeapTupleData, t_bits);

--
Bruce Momjian | 830 Blythe Avenue
maillist(at)candle(dot)pha(dot)pa(dot)us | Drexel Hill, Pennsylvania 19026
+ If your life is a hard drive, | (610) 353-9879(w)
+ Christ can be your backup. | (610) 853-3000(h)

Bruce Momjian wrote:

> Oops, I meant:
>
> len = offsetof(HeapTupleData, t_bits);
>

No luck so far. I am digging around to see if anything has been effected at
all.

At line 812
/* len = sizeof *tuple - sizeof tuple->t_bits; */
len = offsetof(HeapTupleData, t_bits);