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) ***