Re: [PATCHES] page macros cleanup (ver 04)
Zdenek Kotala <[EMAIL PROTECTED]> writes: > Thanks for applying patch. I think that hash index "upgradebility" is > currently broken or it will be with new hash index improvement. But if > I think about it it does not make sense to break compatibility by this > patch first. Right, my point exactly. Those necessarily-incompatible changes might or might not ever get applied --- if they are, we can do cosmetic cleanups afterwards. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup (ver 04)
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 scream bloody murder about that. You're creating a work item for in-place-upgrade that would not otherwise exist, in exchange for a completely trivial bit of code beautification. (The same can be said of his proposed change to hash meta pages.) :-) Yeah, These changes break in-place-upgrade on hash indexes and invokes reindexing request. I have had several reasons why I didn't complaint about it: 1) IIRC, hash function for double has been change 2) there is ongoing project to improve hash index performance -> completely redesigned content 3) hash index is not much used (by my opinion) and it affect only small group of users I'm planning to go over this patch today and apply it sans the parts that would require catversion bump. We can argue later about whether those are really worth doing, but I'm leaning to "not" --- unless Zdenek says that he has no intention of making in-place-upgrade handle hash indexes any time soon. Thanks for applying patch. I think that hash index "upgradebility" is currently broken or it will be with new hash index improvement. But if I think about it it does not make sense to break compatibility by this patch first. I will prepare patch which will be upgrade friendly. And if we will reimplement hash index soon, than we can clean a code. BTW, after further thought about the PageGetContents() situation: right now we can change it to guarantee maxalignment "for free", since SizeOfPageHeaderData happens to be maxaligned on all platforms (this wasn't the case as recently as 8.2). So I'm thinking we should do that. There's at least one place that thinks that PageGetContents is the same as page + SizeOfPageHeaderData, but that's easily fixed. On balance it seems like hidden assumptions about alignment are a bigger risk than assumptions about that offset --- anyone want to argue the contrary? I think it is OK and I seen that you already applied a patch. Thanks Zdenek -- Zdenek Kotala Sun Microsystems Prague, Czech Republic http://sun.com/postgresql -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup (ver 04)
"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 about that. You're creating a work item for in-place-upgrade that would not otherwise exist, in exchange for a completely trivial bit of code beautification. (The same can be said of his proposed change to hash meta pages.) I'm planning to go over this patch today and apply it sans the parts that would require catversion bump. We can argue later about whether those are really worth doing, but I'm leaning to "not" --- unless Zdenek says that he has no intention of making in-place-upgrade handle hash indexes any time soon. BTW, after further thought about the PageGetContents() situation: right now we can change it to guarantee maxalignment "for free", since SizeOfPageHeaderData happens to be maxaligned on all platforms (this wasn't the case as recently as 8.2). So I'm thinking we should do that. There's at least one place that thinks that PageGetContents is the same as page + SizeOfPageHeaderData, but that's easily fixed. On balance it seems like hidden assumptions about alignment are a bigger risk than assumptions about that offset --- anyone want to argue the contrary? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup (ver 04)
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 pointer, so at least in principle this could result in alignment violations. It would accidentally fail to fail as of CVS HEAD, but any future rearrangement of PageHeaderData or HashMetaPageData could create a problem. (Possibly PageGetContents *should* guarantee a maxaligned result, but the current coding does not.) regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup (ver 04)
Heikki Linnakangas napsal(a): Zdenek Kotala wrote: Pavan Deolasee napsal(a): There's another academical discrepancy between these two hunks: 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 portion, without any line pointers, but in HashPageGetBitmap, we're reserving space for one pointer. It looks like the actual size of the bitmap is only the largest power of 2 below the maximum size, so that won't be an issue in practice. 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. 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: #define HashPageGetMeta(page) ((HashMetaPage) (PageGetContents(page))) It also needs bump a catalog version and if we do it now I think it is better to connect both these patches and do bump only once. Attached is an updated patch. I also fixed some whitespace and comments that were no longer valid. How does it look to you now? Perfect. Thanks Zdenek diff -rc pgsql_hash.851ed84fb6aa/src/backend/access/hash/hash.c pgsql_hash/src/backend/access/hash/hash.c *** pgsql_hash.851ed84fb6aa/src/backend/access/hash/hash.c út Ärc 8 21:31:38 2008 --- pgsql_hash/src/backend/access/hash/hash.c út Ärc 8 21:31:38 2008 *** *** 527,533 * each bucket. */ metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE); ! metap = (HashMetaPage) BufferGetPage(metabuf); orig_maxbucket = metap->hashm_maxbucket; orig_ntuples = metap->hashm_ntuples; memcpy(&local_metapage, metap, sizeof(local_metapage)); --- 527,533 * each bucket. */ metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE); ! metap = HashPageGetMeta(BufferGetPage(metabuf)); orig_maxbucket = metap->hashm_maxbucket; orig_ntuples = metap->hashm_ntuples; memcpy(&local_metapage, metap, sizeof(local_metapage)); *** *** 629,635 /* Write-lock metapage and check for split since we started */ metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_WRITE, LH_META_PAGE); ! metap = (HashMetaPage) BufferGetPage(metabuf); if (cur_maxbucket != metap->hashm_maxbucket) { --- 629,635 /* Write-lock metapage and check for split since we started */ metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_WRITE, LH_META_PAGE); ! metap = HashPageGetMeta(BufferGetPage(metabuf)); if (cur_maxbucket != metap->hashm_maxbucket) { diff -rc pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashinsert.c pgsql_hash/src/backend/access/hash/hashinsert.c *** pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashinsert.c út Ärc 8 21:31:38 2008 --- pgsql_hash/src/backend/access/hash/hashinsert.c út Ärc 8 21:31:38 2008 *** *** 69,75 /* Read the metapage */ metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE); ! metap = (HashMetaPage) BufferGetPage(metabuf); /* * Check whether the item can fit on a hash page at all. (Eventually, we --- 69,75 /* Read the metapage */ metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE); ! metap = HashPageGetMeta(BufferGetPage(metabuf)); /* * Check whether the item can fit on a hash page at all. (Eventually, we diff -rc pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashovfl.c pgsql_hash/src/backend/access/hash/hashovfl.c *** pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashovfl.c út Ärc 8 21:31:38 2008 --- pgsql_hash/src/backend/access/hash/hashovfl.c út Ärc 8 21:31:38 2008 *** *** 187,193 _hash_chgbufaccess(rel, metabuf, HASH_NOLOCK, HASH_WRITE); _hash_checkpage(rel, metabuf, LH_META_PAGE); ! metap = (HashMetaPage) BufferGetPage(metabuf); /* start search at hashm_firstfree */ orig_firstfree = metap->hashm_firstfree; --- 187,193 _hash_chgbufaccess(rel, metabuf, HASH_NOLOCK, HASH_WRITE); _hash_checkpage(rel, metabuf, LH_META_PAGE); ! metap = HashPageGetMeta(BufferGetPage(metabuf)); /* start search at hashm_firstfree */ orig_firstfree = metap->hashm_firstfree; *** *** 450,456 /* Read the metapage so we can determine which bitmap page to use */ metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE); ! metap = (HashMetaPage) BufferGetPage(metabuf); /* Identify which bit to set */ ovflbitno = blkno_to_bitno(metap, ovflblkno); --- 450,456 /* Read the metapage so we can determine which bitmap page to use */ metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE); ! metap = HashPageGetMeta(BufferGetPage(metabuf)); /* Identify which bit to set */ ovflbitno = blkno_to_bitno(metap, ovflblkno); diff -rc pgsq
Re: [PATCHES] page macros cleanup (ver 04)
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 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. Ok, fair enough. There's another academical discrepancy between these two hunks: *** src/backend/access/hash/hashpage.c 12 May 2008 00:00:44 - 1.75 --- src/backend/access/hash/hashpage.c 9 Jul 2008 11:30:09 - *** *** 407,413 for (i = _hash_log2(metap->hashm_bsize); i > 0; --i) { if ((1 << i) <= (metap->hashm_bsize - ! (MAXALIGN(sizeof(PageHeaderData)) + MAXALIGN(sizeof(HashPageOpaqueData) break; } --- 407,413 for (i = _hash_log2(metap->hashm_bsize); i > 0; --i) { if ((1 << i) <= (metap->hashm_bsize - ! (MAXALIGN(SizeOfPageHeaderData) + MAXALIGN(sizeof(HashPageOpaqueData) break; } and *** src/include/access/hash.h 19 Jun 2008 00:46:05 - 1.88 --- src/include/access/hash.h 9 Jul 2008 11:30:10 - *** *** 192,198 #define BMPG_SHIFT(metap) ((metap)->hashm_bmshift) #define BMPG_MASK(metap) (BMPGSZ_BIT(metap) - 1) #define HashPageGetBitmap(pg) \ ! ((uint32 *) (((char *) (pg)) + MAXALIGN(sizeof(PageHeaderData /* * The number of bits in an ovflpage bitmap word. --- 191,197 #define BMPG_SHIFT(metap) ((metap)->hashm_bmshift) #define BMPG_MASK(metap) (BMPGSZ_BIT(metap) - 1) #define HashPageGetBitmap(pg) \ ! ((uint32 *) (((char *) (pg)) + MAXALIGN(SizeOfPageHeaderData + sizeof(ItemIdData /* * The number of bits in an ovflpage bitmap word. 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 portion, without any line pointers, but in HashPageGetBitmap, we're reserving space for one pointer. It looks like the actual size of the bitmap is only the largest power of 2 below the maximum size, so that won't be an issue in practice. 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. Attached is an updated patch. I also fixed some whitespace and comments that were no longer valid. How does it look to you now? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com ? DEADJOE ? GNUmakefile ? buildlog ? config.log ? config.status ? page_04-heikki-1.patch ? contrib/pg_standby/.deps ? contrib/pg_standby/pg_standby ? contrib/spi/.deps ? doc/src/sgml/cvsmsg ? src/Makefile.global ? src/backend/postgres ? src/backend/access/common/.deps ? src/backend/access/gin/.deps ? src/backend/access/gist/.deps ? src/backend/access/hash/.deps ? src/backend/access/heap/.deps ? src/backend/access/index/.deps ? src/backend/access/nbtree/.deps ? src/backend/access/transam/.deps ? src/backend/bootstrap/.deps ? src/backend/catalog/.deps ? src/backend/catalog/postgres.bki ? src/backend/catalog/postgres.description ? src/backend/catalog/postgres.shdescription ? src/backend/commands/.deps ? src/backend/executor/.deps ? src/backend/lib/.deps ? src/backend/libpq/.deps ? src/backend/main/.deps ? src/backend/nodes/.deps ? src/backend/optimizer/geqo/.deps ? src/backend/optimizer/path/.deps ? src/backend/optimizer/plan/.deps ? src/backend/optimizer/prep/.deps ? src/backend/optimizer/util/.deps ? src/backend/parser/.deps ? src/backend/port/.deps ? src/backend/postmaster/.deps ? src/backend/regex/.deps ? src/backend/rewrite/.deps ? src/backend/snowball/.deps ? src/backend/snowball/snowball_create.sql ? src/backend/storage/buffer/.deps ? src/backend/storage/file/.deps ? src/backend/storage/freespace/.deps ? src/backend/storage/ipc/.deps ? src/backend/storage/large_object/.deps ? src/backend/storage/lmgr/.deps ? src/backend/storage/page/.deps ? src/backend/storage/smgr/.deps ? src/backend/tcop/.deps ? src/backend/tsearch/.deps ? src/backend/utils/.deps ? src/backend/utils/probes.h ? src/backend/utils/adt/.deps ? src/backend/utils/cache/.deps ? src/backend/utils/error/.deps ? src/backend/utils/fmgr/.deps ? src/backend/utils/hash/.deps ? src/
Re: [PATCHES] page macros cleanup (ver 04)
Pavan, Heikki, Is it OK now or do you have any comments? Thanks Zdenek Zdenek Kotala napsal(a): 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 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup (ver 04)
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 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 diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/gist/gistutil.c pgsql.orig/src/backend/access/gist/gistutil.c *** pgsql.orig.da8c485e0e2a/src/backend/access/gist/gistutil.c pá Ärc 4 14:04:20 2008 --- pgsql.orig/src/backend/access/gist/gistutil.c pá Ärc 4 14:04:20 2008 *** *** 592,599 /* * Additionally check that the special area looks sane. */ ! if (((PageHeader) (page))->pd_special != ! (BLCKSZ - MAXALIGN(sizeof(GISTPageOpaqueData ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", --- 592,598 /* * Additionally check that the special area looks sane. */ ! if ( PageGetSpecialSize(page) != MAXALIGN(sizeof(GISTPageOpaqueData))) ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashpage.c pgsql.orig/src/backend/access/hash/hashpage.c *** pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashpage.c pá Ärc 4 14:04:20 2008 --- pgsql.orig/src/backend/access/hash/hashpage.c pá Ärc 4 14:04:20 2008 *** *** 407,413 for (i = _hash_log2(metap->hashm_bsize); i > 0; --i) { if ((1 << i) <= (metap->hashm_bsize - ! (MAXALIGN(sizeof(PageHeaderData)) + MAXALIGN(sizeof(HashPageOpaqueData) break; } --- 407,413 for (i = _hash_log2(metap->hashm_bsize); i > 0; --i) { if ((1 << i) <= (metap->hashm_bsize - ! (MAXALIGN(SizeOfPageHeaderData) + MAXALIGN(sizeof(HashPageOpaqueData) break; } diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashutil.c pgsql.orig/src/backend/access/hash/hashutil.c *** pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashutil.c pá Ärc 4 14:04:20 2008 --- pgsql.orig/src/backend/access/hash/hashutil.c pá Ärc 4 14:04:20 2008 *** *** 164,171 /* * Additionally check that the special area looks sane. */ ! if (((PageHeader) (page))->pd_special != ! (BLCKSZ - MAXALIGN(sizeof(HashPageOpaqueData ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", --- 164,170 /* * Additionally check that the special area looks sane. */ ! if ( PageGetSpecialSize(page) != MAXALIGN(sizeof(HashPageOpaqueData))) ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/heap/heapam.c pgsql.orig/src/backend/access/heap/heapam.c *** pgsql.orig.da8c485e0e2a/src/backend/access/heap/heapam.c pá Ärc 4 14:04:20 2008 --- pgsql.orig/src/backend/access/heap/heapam.c pá Ärc 4 14:04:20 2008 *** *** 1342,1348 ItemPointer tid = &(tuple->t_self); ItemId lp; Buffer buffer; ! PageHeader dp; OffsetNumber offnum; bool valid; --- 1342,1348 ItemPointer tid = &(tuple->t_self); ItemId lp; Buffer buffer; ! Page page; OffsetNumber offnum; bool valid; *** *** 1355,1361 * Need share lock on buffer to examine tuple commit status. */ LockBuffer(buffer, BUFFER_LOCK_SHARE); ! dp = (PageHeader) BufferGetPage(buffer); /* * We'd better check for out-of-range offnum in case of VACUUM since the --- 1355,1361 * Need share lock on buffer to examine tuple commit status. */ LockBuffer(buffer, BUFFER_LOCK_SHARE); ! page = BufferGetPage(buffer); /* * We'd better check for out-of-range offnum in case of VACUUM since the *** *** 1362,1368 * TID was obtained. */ offnum = ItemPointerGetOffsetNumber(tid); ! if (offnum < FirstOffsetNumber || offnum > PageGetMaxOffsetNumber(dp)) { LockBuffer(buffer, BUFFER_LOCK_UNLOCK); if (keep_buf) --- 1362,1368 * TID was obtained. */ offnum = ItemPointerGetOffsetNumber(tid); ! if (offnum < FirstOffsetNumber || offnum > PageGetMaxOffsetNumber(page)) { LockBuffer(buffer, BUFFER_LOCK_UNLOCK); if (keep_buf) ***
Re: [PATCHES] page macros cleanup
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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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 from offset point of view which is opposite to insert order. Zdenek -- Zdenek Kotala Sun Microsystems Prague, Czech Republic http://sun.com/postgresql -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
Heikki Linnakangas napsal(a): 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)) 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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 http://www.enterprisedb.com -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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 academical though, so not a big deal. Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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)) 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup (ver 03)
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 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 Orig81448144 New 81488144 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 diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/gist/gistutil.c pgsql.orig/src/backend/access/gist/gistutil.c *** pgsql.orig.da8c485e0e2a/src/backend/access/gist/gistutil.c pá Ärc 4 10:23:41 2008 --- pgsql.orig/src/backend/access/gist/gistutil.c pá Ärc 4 10:23:41 2008 *** *** 592,599 /* * Additionally check that the special area looks sane. */ ! if (((PageHeader) (page))->pd_special != ! (BLCKSZ - MAXALIGN(sizeof(GISTPageOpaqueData ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", --- 592,598 /* * Additionally check that the special area looks sane. */ ! if ( PageGetSpecialSize(page) != MAXALIGN(sizeof(GISTPageOpaqueData))) ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashpage.c pgsql.orig/src/backend/access/hash/hashpage.c *** pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashpage.c pá Ärc 4 10:23:41 2008 --- pgsql.orig/src/backend/access/hash/hashpage.c pá Ärc 4 10:23:41 2008 *** *** 407,413 for (i = _hash_log2(metap->hashm_bsize); i > 0; --i) { if ((1 << i) <= (metap->hashm_bsize - ! (MAXALIGN(sizeof(PageHeaderData)) + MAXALIGN(sizeof(HashPageOpaqueData) break; } --- 407,413 for (i = _hash_log2(metap->hashm_bsize); i > 0; --i) { if ((1 << i) <= (metap->hashm_bsize - ! (MAXALIGN(SizeOfPageHeaderData) + MAXALIGN(sizeof(HashPageOpaqueData) break; } diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashutil.c pgsql.orig/src/backend/access/hash/hashutil.c *** pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashutil.c pá Ärc 4 10:23:41 2008 --- pgsql.orig/src/backend/access/hash/hashutil.c pá Ärc 4 10:23:41 2008 *** *** 164,171 /* * Additionally check that the special area looks sane. */ ! if (((PageHeader) (page))->pd_special != ! (BLCKSZ - MAXALIGN(sizeof(HashPageOpaqueData ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", --- 164,170 /* * Additionally check that the special area looks sane. */ ! if ( PageGetSpecialSize(page) != MAXALIGN(sizeof(HashPageOpaqueData))) ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/heap/heapam.c pgsql.orig/src/backend/access/heap/heapam.c *** pgsql.orig.da8c485e0e2a/src/backend/access/heap/heapam.c pá Ärc 4 10:23:41 2008 --- pgsql.orig/src/backend/access/heap/heapam.c pá Ärc 4 10:23:41 2008 *** *** 1342,1348 ItemPointer tid = &(tuple->t_self); ItemId lp; Buffer buffer; ! PageHeader dp; OffsetNumber offnum; bool valid; --- 1342,1348 ItemPointer tid = &(tuple->t_self); ItemId lp; Buffer buffer; ! Page page; OffsetNumber offnum; bool valid; *** *** 1355,1361 * Need share lock on buffer to examine tuple commit status. */ LockBuffer(buffer, BUFFER_LOCK_SHARE); ! dp = (PageHeader) BufferGetPage(buffer); /* * We'd better check for out-of-range offnum in case of VACUUM since the --- 1355,1361 * Need share lock on buffer to examine tuple commit status. */ LockBuffer(buffer, BUFFER_LOCK_SHARE); ! page = BufferGetPage(buffer); /* * We'd better check for out-of-range offnum in case of VACUUM since the *** *** 1362,1368 * TID was obtained. */ offnum = ItemPointerGetOffsetNumber(tid); ! if (offnum < FirstOffsetNumber || offnum > PageGetMaxOffsetNumber(dp)) { LockBuffer(buffer, BUFFER_LOCK_UNLOCK); if (keep_buf) --- 1362,1368 * TID was obtained. */ offnum = ItemPointerGetOffsetNumber(tid); ! if (offnum < FirstOffsetNumber || offnum > PageGetMaxOffsetNumber(page)) { LockBuffer(buffer, BUFFER_LOCK_UNLOCK); if (keep_buf)
Re: [PATCHES] page macros cleanup
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 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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 diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/gist/gistutil.c pgsql.orig/src/backend/access/gist/gistutil.c *** pgsql.orig.da8c485e0e2a/src/backend/access/gist/gistutil.c pá Ärc 4 10:23:41 2008 --- pgsql.orig/src/backend/access/gist/gistutil.c pá Ärc 4 10:23:41 2008 *** *** 592,599 /* * Additionally check that the special area looks sane. */ ! if (((PageHeader) (page))->pd_special != ! (BLCKSZ - MAXALIGN(sizeof(GISTPageOpaqueData ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", --- 592,598 /* * Additionally check that the special area looks sane. */ ! if ( PageGetSpecialSize(page) != MAXALIGN(sizeof(GISTPageOpaqueData))) ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashpage.c pgsql.orig/src/backend/access/hash/hashpage.c *** pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashpage.c pá Ärc 4 10:23:41 2008 --- pgsql.orig/src/backend/access/hash/hashpage.c pá Ärc 4 10:23:41 2008 *** *** 407,413 for (i = _hash_log2(metap->hashm_bsize); i > 0; --i) { if ((1 << i) <= (metap->hashm_bsize - ! (MAXALIGN(sizeof(PageHeaderData)) + MAXALIGN(sizeof(HashPageOpaqueData) break; } --- 407,413 for (i = _hash_log2(metap->hashm_bsize); i > 0; --i) { if ((1 << i) <= (metap->hashm_bsize - ! (MAXALIGN(SizeOfPageHeaderData) + MAXALIGN(sizeof(HashPageOpaqueData) break; } diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashutil.c pgsql.orig/src/backend/access/hash/hashutil.c *** pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashutil.c pá Ärc 4 10:23:41 2008 --- pgsql.orig/src/backend/access/hash/hashutil.c pá Ärc 4 10:23:41 2008 *** *** 164,171 /* * Additionally check that the special area looks sane. */ ! if (((PageHeader) (page))->pd_special != ! (BLCKSZ - MAXALIGN(sizeof(HashPageOpaqueData ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", --- 164,170 /* * Additionally check that the special area looks sane. */ ! if ( PageGetSpecialSize(page) != MAXALIGN(sizeof(HashPageOpaqueData))) ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" contains corrupted page at block %u", diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/heap/heapam.c pgsql.orig/src/backend/access/heap/heapam.c *** pgsql.orig.da8c485e0e2a/src/backend/access/heap/heapam.c pá Ärc 4 10:23:41 2008 --- pgsql.orig/src/backend/access/heap/heapam.c pá Ärc 4 10:23:41 2008 *** *** 1342,1348 ItemPointer tid = &(tuple->t_self); ItemId lp; Buffer buffer; ! PageHeader dp; OffsetNumber offnum; bool valid; --- 1342,1348 ItemPointer tid = &(tuple->t_self); ItemId lp; Buffer buffer; ! Page page; OffsetNumber offnum; bool valid; *** *** 1355,1361 * Need share lock on buffer to examine tuple commit status. */ LockBuffer(buffer, BUFFER_LOCK_SHARE); ! dp = (PageHeader) BufferGetPage(buffer); /* * We'd better check for out-of-range offnum in case of VACUUM since the --- 1355,1361 * Need share lock on buffer to examine tuple commit status. */ LockBuffer(buffer, BUFFER_LOCK_SHARE); ! page = BufferGetPage(buffer); /* * We'd better check for out-of-range offnum in case of VACUUM since the *** *** 1362,1368 * TID was obtained. */ offnum = ItemPointerGetOffsetNumber(tid); ! if (offnum < FirstOffsetNumber || offnum > PageGetMaxOffsetNumber(dp)) { LockBuffer(buffer, BUFFER_LOCK_UNLOCK); if (keep_buf) --- 1362,1368 * TID was obtained. */ offnum = ItemPointerGetOffsetNumber(tid); ! if (offnum < FirstOffsetNumber || offnum > PageGetMaxOffsetNumber(page)) { LockBuffer(buffer, BUFFER_LOCK_UNLOCK); if (keep_buf) *** *** 1379,1385 /* * get the item line pointer corresponding to the requested tid */ ! lp = PageGetItemId(dp, offnum); /* * Must check for deleted tuple.
Re: [PATCHES] page macros cleanup
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 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] page macros cleanup
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 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] page macros cleanup
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. There still lot of places which directly access to PageHeader structure which require new macros/fuction in page API. I will fix it in next patch. Zdenek diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/gist/gistutil.c pgsql.orig/src/backend/access/gist/gistutil.c *** 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), diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashpage.c pgsql.orig/src/backend/access/hash/hashpage.c *** pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashpage.c pá Ärn 13 18:00:35 2008 --- pgsql.orig/src/backend/access/hash/hashpage.c pá Ärn 13 18:00:35 2008 *** *** 407,413 for (i = _hash_log2(metap->hashm_bsize); i > 0; --i) { if ((1 << i) <= (metap->hashm_bsize - ! (MAXALIGN(sizeof(PageHeaderData)) + MAXALIGN(sizeof(HashPageOpaqueData) break; } --- 407,413 for (i = _hash_log2(metap->hashm_bsize); i > 0; --i) { if ((1 << i) <= (metap->hashm_bsize - ! (MAXALIGN(SizeOfPageHeaderData) + MAXALIGN(sizeof(HashPageOpaqueData) break; } diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashutil.c pgsql.orig/src/backend/access/hash/hashutil.c *** pgsql.orig.da8c485e0e2a/src/backend/access/hash/hashutil.c pá Ärn 13 18:00:35 2008 --- pgsql.orig/src/backend/access/hash/hashutil.c pá Ärn 13 18:00:35 2008 *** *** 164,170 /* * Additionally check that the special area looks sane. */ ! if (((PageHeader) (page))->pd_special != (BLCKSZ - MAXALIGN(sizeof(HashPageOpaqueData ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), --- 164,170 /* * Additionally check that the special area looks sane. */ ! if ( PageGetSpecialPointer(page) - page != (BLCKSZ - MAXALIGN(sizeof(HashPageOpaqueData ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), diff -cr pgsql.orig.da8c485e0e2a/src/backend/access/heap/heapam.c pgsql.orig/src/backend/access/heap/heapam.c *** pgsql.orig.da8c485e0e2a/src/backend/access/heap/heapam.c pá Ärn 13 18:00:35 2008 --- pgsql.orig/src/backend/access/heap/heapam.c pá Ärn 13 18:00:35 2008 *** *** 1342,1348 ItemPointer tid = &(tuple->t_self); ItemId lp; Buffer buffer; ! PageHeader dp; OffsetNumber offnum; bool valid; --- 1342,1348 ItemPointer tid = &(tuple->t_self); ItemId lp; Buffer buffer; ! Page page; OffsetNumber offnum; bool valid; *** *** 1355,1361 * Need share lock on buffer to examine tuple commit status. */ LockBuffer(buffer, BUFFER_LOCK_SHARE); ! dp = (PageHeader) BufferGetPage(buffer); /* * We'd better check for out-of-range offnum in case of VACUUM since the --- 1355,1361 * Need share lock on buffer to examine tuple commit status. */ LockBuffer(buffer, BUFFER_LOCK_SHARE); ! page = BufferGetPage(buffer); /* * We'd better check for out-of-range offnum in case of VACUUM since the *** *** 1362,1368 * TID was obtained. */ offnum = ItemPointerGetOffsetNumber(tid); ! if (offnum < FirstOffsetNumber || offnum > PageGetMaxOffsetNumber(dp)) { LockBuffer(buffer, BUFFER_LOCK_UNLOCK); if (keep_buf) --- 1362,1368 * TID was obtained. */ offnum = ItemPointerGetOffsetNumber(tid); ! if (offnum < FirstOffsetNumber || offnum > PageGetMaxOffsetNumber(page)) { LockBuffer(buffer, BUFFER_LOCK_UNLOCK); if (keep_buf) *** *** 1379,1385 /* * get the item line pointer corresponding to the requested tid */ ! lp = PageGetItemId(dp, offnum); /* * Must check for deleted tuple. --- 1379,1385 /* * get the item line pointer corresponding to the requested tid */ ! lp = PageGetItemId(page, offnum); /* * Must check for deleted tuple. *** *** 1401,1407 /* * fill in *tuple fields */ ! tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lp); tuple->t_len = ItemIdGetLength(lp); tuple->t_tableOid = RelationGetRelid(relation); --- 1401,1407 /* * fill in *tuple fields */ ! tuple->t_data = (HeapTupleHe