page macros cleanup

On Fri, Jun 13, 2008 at 9:38 PM, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:

I attached code cleanup which is related to in-place upgrade. I replace
direct access to PageHeader structure with already existing macros and I
removed also unnecessary retyping.

A quick review comment:

One thing I noticed is that the modified definition of HashMaxItemSize
now does not account for the size of ItemId which may not be the right
thing. Please recheck that.

Apart from that the patch looks good to me. As you said, we should
probably replace the other direct usage of PageHeader with appropriate
macros.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

Just one quick note:

Zdenek Kotala wrote:

*** pgsql.orig.da8c485e0e2a/src/backend/access/gist/gistutil.c	pá črn 13 18:00:35 2008
--- pgsql.orig/src/backend/access/gist/gistutil.c	pá črn 13 18:00:35 2008
***************
*** 592,598 ****
/*
* Additionally check that the special area looks sane.
*/
! 	if (((PageHeader) (page))->pd_special !=
(BLCKSZ - MAXALIGN(sizeof(GISTPageOpaqueData))))
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
--- 592,598 ----
/*
* Additionally check that the special area looks sane.
*/
! 	if ( PageGetSpecialPointer(page) - page !=
(BLCKSZ - MAXALIGN(sizeof(GISTPageOpaqueData))))
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),

Should probably use PageGetSpecialSize here. Much simpler, and doesn't
assume that the special area is always at the end of page (not that I
see us changing that anytime soon).

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Pavan Deolasee napsal(a):

On Fri, Jun 13, 2008 at 9:38 PM, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:

I attached code cleanup which is related to in-place upgrade. I replace
direct access to PageHeader structure with already existing macros and I
removed also unnecessary retyping.

A quick review comment:

Thanks you for your review,

One thing I noticed is that the modified definition of HashMaxItemSize
now does not account for the size of ItemId which may not be the right
thing. Please recheck that.

Good catch. I lost in basic arithmetic. What I see now that original definition
count sizeof(ItemIdData) twice and on other side it does not take care about
MAXALING correctly. I think correct formula is:

#define HashMaxItemSize(page) \
(PageGetPageSize(page) - \
( MAXALIGN(SizeOfPageHeaderData + sizeof(ItemIdData))+ \
MAXALIGN(sizeof(HashPageOpaqueData)) \
)\
)

What do you think?

Thanks for your comments Zdenek

--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql

Heikki Linnakangas napsal(a):

Just one quick note:

Should probably use PageGetSpecialSize here. Much simpler, and doesn't
assume that the special area is always at the end of page (not that I
see us changing that anytime soon).

Thanks for review,
good catch. I found similar example also in nbtree and hash. I attached updated
patch version which also contains fix for Pavan's observation. Patch modifies
HashMaxItemSize. It should require reindex on all hash indexes. Should we bump
catalog version?

thanks Zdenek

--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql

On Fri, Jul 4, 2008 at 1:01 PM, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:

Good catch. I lost in basic arithmetic. What I see now that original
definition count sizeof(ItemIdData) twice and on other side it does not take
care about MAXALING correctly. I think correct formula is:

#define HashMaxItemSize(page) \
(PageGetPageSize(page) - \
( MAXALIGN(SizeOfPageHeaderData + sizeof(ItemIdData))+ \
MAXALIGN(sizeof(HashPageOpaqueData)) \
)\
)

What do you think?

Yes. I think that's the correct way.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

On Fri, Jul 4, 2008 at 2:08 PM, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:

Patch
modifies HashMaxItemSize. It should require reindex on all hash indexes.
Should we bump catalog version?

Do we really need that ? Even if the new HashMaxItemSize comes out to
be lower than the current limit (because of MAXALIGN), I don't see how
existing hash indexes can have a larger item than the new limit.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

Pavan Deolasee napsal(a):

On Fri, Jul 4, 2008 at 2:08 PM, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:

Patch
modifies HashMaxItemSize. It should require reindex on all hash indexes.
Should we bump catalog version?

Do we really need that ? Even if the new HashMaxItemSize comes out to
be lower than the current limit (because of MAXALIGN), I don't see how
existing hash indexes can have a larger item than the new limit.

Yeah, I performed calculation and situation is following (for MAXALIGN 4 and 8):

4 8
Orig 8144 8144
New 8148 8144

It should be OK for all cases.

I fixed also one typo (ItemId -> ItemIdData) and new revision is attached.

Zdenek

--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql

Pavan Deolasee wrote:

On Fri, Jul 4, 2008 at 1:01 PM, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:

Good catch. I lost in basic arithmetic. What I see now that original
definition count sizeof(ItemIdData) twice and on other side it does not take
care about MAXALING correctly. I think correct formula is:

#define HashMaxItemSize(page) \
(PageGetPageSize(page) - \
( MAXALIGN(SizeOfPageHeaderData + sizeof(ItemIdData))+ \
MAXALIGN(sizeof(HashPageOpaqueData)) \
)\
)

What do you think?

Yes. I think that's the correct way.

Doesn't look right to me. There's no padding after the first line
pointer, hence the first MAXALIGN shouldn't be there.

BTW, looking at hashinsert.c where it's used, we're actually passing a
pointer to the meta page to HashMaxItemSize(). So the PageGetPageSize()
call on that is quite bogus, since it's not the meta page that the tuple
is going to be inserted to. It's academical, because all pages are the
same size anyway, but doesn't look right. I think I'd go with BLKCSZ
instead.

I think this is the way it should be:

#define HashMaxItemSize \
(BLCKSZ - \
SizeOfPageHeaderData - \
MAXALIGN(sizeof(HashPageOpaqueData)) - \
sizeof(ItemIdData))

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Fri, Jul 4, 2008 at 3:37 PM, Heikki Linnakangas
<heikki@enterprisedb.com> wrote:

I think this is the way it should be:

#define HashMaxItemSize \
(BLCKSZ - \
SizeOfPageHeaderData - \
MAXALIGN(sizeof(HashPageOpaqueData)) - \
sizeof(ItemIdData))

I am wondering if this would fail for corner case if HashMaxItemSize
happened to be unaligned. For example, if (itemsz < HashMaxItemSize <
MAXALIGN(itemsz), PageAddItem() would later fail with a not-so-obvious
error. Should we just MAXALIGN_DOWN the HashMaxItemSize ?

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas napsal(a):

Pavan Deolasee wrote:

On Fri, Jul 4, 2008 at 1:01 PM, Zdenek Kotala <Zdenek.Kotala@sun.com>
wrote:

Good catch. I lost in basic arithmetic. What I see now that original
definition count sizeof(ItemIdData) twice and on other side it does
not take
care about MAXALING correctly. I think correct formula is:

#define HashMaxItemSize(page) \
(PageGetPageSize(page) - \
( MAXALIGN(SizeOfPageHeaderData + sizeof(ItemIdData))+ \
MAXALIGN(sizeof(HashPageOpaqueData)) \
)\
)

What do you think?

Yes. I think that's the correct way.

Doesn't look right to me. There's no padding after the first line
pointer, hence the first MAXALIGN shouldn't be there.

Are you sure? I expecting that tupleheader must be aligned to MAXALIGN. If it is
not required than you are right.

Look on PageAddItem:

00226 alignedSize = MAXALIGN(size);
00227
00228 upper = (int) phdr->pd_upper - (int) alignedSize;

By my opinion first place where tuple should be placed is:

MAXALIGN(SizeOfPageHeaderData + sizeof(ItemIdData))

BTW, looking at hashinsert.c where it's used, we're actually passing a
pointer to the meta page to HashMaxItemSize(). So the PageGetPageSize()
call on that is quite bogus, since it's not the meta page that the tuple
is going to be inserted to. It's academical, because all pages are the
same size anyway, but doesn't look right. I think I'd go with BLKCSZ
instead.

Yeah, BLKCSZ looks good. Anyway, I plan to reorganize all *MaxItemSize staff to
be compatible with in-place upgrade.

thanks Zdenek

--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql

Pavan Deolasee wrote:

On Fri, Jul 4, 2008 at 3:37 PM, Heikki Linnakangas
<heikki@enterprisedb.com> wrote:

I think this is the way it should be:

#define HashMaxItemSize \
(BLCKSZ - \
SizeOfPageHeaderData - \
MAXALIGN(sizeof(HashPageOpaqueData)) - \
sizeof(ItemIdData))

I am wondering if this would fail for corner case if HashMaxItemSize
happened to be unaligned. For example, if (itemsz < HashMaxItemSize <
MAXALIGN(itemsz), PageAddItem() would later fail with a not-so-obvious
error. Should we just MAXALIGN_DOWN the HashMaxItemSize ?

No, there's a itemsz = MAXALIGN(itemsz) call before the check against
HashMaxItemSize.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Fri, Jul 4, 2008 at 4:25 PM, Heikki Linnakangas
<heikki@enterprisedb.com> wrote:

No, there's a itemsz = MAXALIGN(itemsz) call before the check against
HashMaxItemSize.

Ah, right. Still should we just not MAXALIGN_DOWN the Max size to
reflect the practical limit on the itemsz ? It's more academical
though, so not a big deal.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas napsal(a):

Pavan Deolasee wrote:

On Fri, Jul 4, 2008 at 3:37 PM, Heikki Linnakangas
<heikki@enterprisedb.com> wrote:

I think this is the way it should be:

#define HashMaxItemSize \
(BLCKSZ - \
SizeOfPageHeaderData - \
MAXALIGN(sizeof(HashPageOpaqueData)) - \
sizeof(ItemIdData))

I am wondering if this would fail for corner case if HashMaxItemSize
happened to be unaligned. For example, if (itemsz < HashMaxItemSize <
MAXALIGN(itemsz), PageAddItem() would later fail with a not-so-obvious
error. Should we just MAXALIGN_DOWN the HashMaxItemSize ?

No, there's a itemsz = MAXALIGN(itemsz) call before the check against
HashMaxItemSize.

It is tru, but id somebody will use HashMaxItemSize in different place in the
future he could omit to use same approach. See tuptoaster.h how TOAST_MAX_CHUNK
is defined or BTMaxItemSize in nbtree.h.

Pavan's comments is correct. It should give same result as my implementation
(because BLCKSZ is aligned), but it is better readable and consistent with other.

Zdenek

--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql

On Fri, Jul 4, 2008 at 4:20 PM, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:

By my opinion first place where tuple should be placed is:

MAXALIGN(SizeOfPageHeaderData + sizeof(ItemIdData))

Tuple actually starts from the other end of the block.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

Pavan Deolasee napsal(a):

On Fri, Jul 4, 2008 at 4:20 PM, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:

By my opinion first place where tuple should be placed is:

MAXALIGN(SizeOfPageHeaderData + sizeof(ItemIdData))

Tuple actually starts from the other end of the block.

Yes, but I meant first from offset point of view which is opposite to insert order.

Zdenek

--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql

Zdenek Kotala wrote:

By my opinion first place where tuple should be placed is:

MAXALIGN(SizeOfPageHeaderData + sizeof(ItemIdData))

Yeah, but we don't enforce that directly. We enforce it by MAXALIGNing
size in PageAddItem, and with the rule that special-size is MAXALIGNed.

To put it another way, it's possible that there's an unaligned amount of
free space on a page, as returned by PageGetExactFreeSpace. But since
item size is always MAXALIGNed, it's impossible to use it all.

Come to think of it, why do we require MAXALIGN alignment of tuples? I
must be missing something, but AFAICS the widest fields in
HeapTupleHeaderData are 4-bytes wide, so it should be possible to get
away with 4-byte alignment.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas napsal(a):

Zdenek Kotala wrote:

By my opinion first place where tuple should be placed is:

MAXALIGN(SizeOfPageHeaderData + sizeof(ItemIdData))

Yeah, but we don't enforce that directly. We enforce it by MAXALIGNing
size in PageAddItem, and with the rule that special-size is MAXALIGNed.

Yeah, it is implication from other rules. But how Pavan mentioned and I agree
with him. We should have *MaxItemSize value maxaligned, because tuple is
maxaligned anyway and *MaxItemSize defines real limit.

Come to think of it, why do we require MAXALIGN alignment of tuples? I
must be missing something, but AFAICS the widest fields in
HeapTupleHeaderData are 4-bytes wide, so it should be possible to get
away with 4-byte alignment.

I think that you have problem to make correct decision about padding between
header and first data item on platform where MAXALIGN is 8. Padding will depends
on align of offset - if it is max aligned or not. It will have effect for
example on SPARC but it adds extra complexity to code. ... Maybe only set hoff
correctly when new tuple is created ...

Another improvement should be reorder column to got best space allocation during
create table (of course it affect select * from).

Zdenek

--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql

Pavan Deolasee napsal(a):

On Fri, Jul 4, 2008 at 4:25 PM, Heikki Linnakangas
<heikki@enterprisedb.com> wrote:

No, there's a itemsz = MAXALIGN(itemsz) call before the check against
HashMaxItemSize.

Ah, right. Still should we just not MAXALIGN_DOWN the Max size to
reflect the practical limit on the itemsz ? It's more academical
though, so not a big deal.

Finally I use following formula:

#define HashMaxItemSize(page) \
MAXALIGN_DOWN(PageGetPageSize(page) - \
( SizeOfPageHeaderData + sizeof(ItemIdData) ) - \
MAXALIGN(sizeof(HashPageOpaqueData)) )

I did not replace PageGetPageSize(page), because other *MaxItemSize has same
interface.

Revised patch is attached.

Zdenek

--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql

Pavan, Heikki,

Is it OK now or do you have any comments?

Thanks Zdenek

Zdenek Kotala napsal(a):