Обсуждение: Two different defs of MAX_TUPLES_PER_PAGE

Hi,

I found two different definitions of MAX_TUPLES_PER_PAGE.
Which is reasonable? Or do they have another meaning?

backend/commands/vacuumlazy.c
#define MAX_TUPLES_PER_PAGE  ((int) (BLCKSZ / sizeof(HeapTupleHeaderData)))

backend/nodes/tidbitmap.c
#define MAX_TUPLES_PER_PAGE  ((BLCKSZ - 1) / MAXALIGN(offsetof(HeapTupleHeaderData, t_bits) + sizeof(ItemIdData)) + 1)

---
ITAGAKI Takahiro
NTT Cyber Space Laboratories

ITAGAKI Takahiro <itagaki.takahiro@lab.ntt.co.jp> writes:
> I found two different definitions of MAX_TUPLES_PER_PAGE.
> Which is reasonable? Or do they have another meaning?

Hmm, I think those were both my fault at different times :-(.
Right now I am thinking that they are both not quite right,
in particular it ought to be

#define MAX_TUPLES_PER_PAGE  ((BLCKSZ - 1) / (MAXALIGN(offsetof(HeapTupleHeaderData, t_bits)) + sizeof(ItemIdData)) +
1)

That is, the heaptuple space is padded to a MAXALIGN boundary, but the
itemid that points to it isn't.  Comments?

(I believe that both modules want a ceiling definition not a floor
definition, ie round up any fraction.  The -1 / +1 trick is of course
just one way to get that.)

Also, is this something that should be in a common header file?  If so
which one?  BLCKSZ, HeapTupleHeaderData, and ItemIdData are all defined
in different places ...
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> #define MAX_TUPLES_PER_PAGE  ((BLCKSZ - 1) / (MAXALIGN(offsetof(HeapTupleHeaderData, t_bits)) + sizeof(ItemIdData)) +
1)
> (I believe that both modules want a ceiling definition not a floor
> definition, ie round up any fraction.  The -1 / +1 trick is of course
> just one way to get that.)

Don't you think about PageHeaderData? Also I guess a floor definition is ok
because 'number of tuples' is an integer. How about the following?

((BLCKSZ - offsetof(PageHeaderData, pd_linp)) /   (MAXALIGN(offsetof(HeapTupleHeaderData, t_bits)) +
sizeof(ItemIdData)))


> Also, is this something that should be in a common header file?  If so
> which one?  BLCKSZ, HeapTupleHeaderData, and ItemIdData are all defined
> in different places ...

Considering include-hierarchy, I think bufpage.h is a good place.

---
ITAGAKI Takahiro
NTT Cyber Space Laboratories

ITAGAKI Takahiro <itagaki.takahiro@lab.ntt.co.jp> writes:
> Don't you think about PageHeaderData?

I doubt it's really worth taking into account ... we can though.

> Also I guess a floor definition is ok
> because 'number of tuples' is an integer.

Right, now that I'm more awake I agree with that ;-)

>> Also, is this something that should be in a common header file?  If so
>> which one?  BLCKSZ, HeapTupleHeaderData, and ItemIdData are all defined
>> in different places ...

> Considering include-hierarchy, I think bufpage.h is a good place.

No, that's a pretty bad place because it violates the module hierarchy:
access is on top of storage.  None of the include/storage files know
what a HeapTupleHeader looks like.

I think we can just put it in htup.h, since that includes bufpage.h
already.  Will make it happen.
        regards, tom lane