Re: [PATCHES] page macros cleanup (ver 04)

2008-07-21 Thread Zdenek Kotala
Tom Lane napsal(a): Heikki Linnakangas [EMAIL PROTECTED] writes: ... That macro is actually doing the same thing as PageGetContents, so I switched to using that. As that moves the data sligthly on those bitmap pages, I guess we'll need a catversion bump. I'm amazed that Zdenek didn't

Re: [PATCHES] page macros cleanup (ver 04)

2008-07-13 Thread Tom Lane
Zdenek Kotala [EMAIL PROTECTED] writes: Good catch. if we have to bump catalog version then I have there small patch which introduce following macro and remove PageHeaderData stucture from HashMetaPageData: Seems like a bad idea --- PageGetContents is not guaranteed to deliver a maxaligned

Re: [PATCHES] page macros cleanup (ver 04)

2008-07-13 Thread Tom Lane
Heikki Linnakangas [EMAIL PROTECTED] writes: ... That macro is actually doing the same thing as PageGetContents, so I switched to using that. As that moves the data sligthly on those bitmap pages, I guess we'll need a catversion bump. I'm amazed that Zdenek didn't scream bloody murder

Re: [PATCHES] page macros cleanup (ver 04)

2008-07-09 Thread Heikki Linnakangas
Zdenek Kotala wrote: Pavan Deolasee napsal(a): On Fri, Jul 4, 2008 at 4:25 PM, Heikki Linnakangas [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup (ver 04)

2008-07-09 Thread Zdenek Kotala
Heikki Linnakangas napsal(a): Zdenek Kotala wrote: Pavan Deolasee napsal(a): There's another academical discrepancy between these two hunks: snip I admit I don't understand what that bitmap is, it looks like we're assuming it can take up all the space between page header and the special

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Zdenek Kotala
Pavan Deolasee napsal(a): On Fri, Jun 13, 2008 at 9:38 PM, Zdenek Kotala [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Zdenek Kotala
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

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Pavan Deolasee
On Fri, Jul 4, 2008 at 1:01 PM, Zdenek Kotala [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Pavan Deolasee
On Fri, Jul 4, 2008 at 2:08 PM, Zdenek Kotala [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup (ver 03)

2008-07-04 Thread Zdenek Kotala
Pavan Deolasee napsal(a): On Fri, Jul 4, 2008 at 2:08 PM, Zdenek Kotala [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Heikki Linnakangas
Pavan Deolasee wrote: On Fri, Jul 4, 2008 at 1:01 PM, Zdenek Kotala [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Pavan Deolasee
On Fri, Jul 4, 2008 at 3:37 PM, Heikki Linnakangas [EMAIL PROTECTED] wrote: I think this is the way it should be: #define HashMaxItemSize \ (BLCKSZ - \ SizeOfPageHeaderData - \ MAXALIGN(sizeof(HashPageOpaqueData)) - \ sizeof(ItemIdData)) I am wondering if

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Zdenek Kotala
Heikki Linnakangas napsal(a): Pavan Deolasee wrote: On Fri, Jul 4, 2008 at 1:01 PM, Zdenek Kotala [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Heikki Linnakangas
Pavan Deolasee wrote: On Fri, Jul 4, 2008 at 3:37 PM, Heikki Linnakangas [EMAIL PROTECTED] wrote: I think this is the way it should be: #define HashMaxItemSize \ (BLCKSZ - \ SizeOfPageHeaderData - \ MAXALIGN(sizeof(HashPageOpaqueData)) - \ sizeof(ItemIdData))

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Pavan Deolasee
On Fri, Jul 4, 2008 at 4:25 PM, Heikki Linnakangas [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Pavan Deolasee
On Fri, Jul 4, 2008 at 4:20 PM, Zdenek Kotala [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Zdenek Kotala
Pavan Deolasee napsal(a): On Fri, Jul 4, 2008 at 4:20 PM, Zdenek Kotala [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup

2008-07-04 Thread Heikki Linnakangas
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

Re: [PATCHES] page macros cleanup (ver 04)

2008-07-04 Thread Zdenek Kotala
Pavan Deolasee napsal(a): On Fri, Jul 4, 2008 at 4:25 PM, Heikki Linnakangas [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup

2008-07-03 Thread Pavan Deolasee
On Fri, Jun 13, 2008 at 9:38 PM, Zdenek Kotala [EMAIL PROTECTED] 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

Re: [PATCHES] page macros cleanup

2008-07-03 Thread Heikki Linnakangas
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