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)
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 [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 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
?
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:
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 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
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)
***
*** 1379,1385
/*
* get the
