[PATCH] PageGetTempPage cleanup
I attach patch which cleans up code around PageGetTempPage. These changes were
discussed here:
http://archives.postgresql.org/pgsql-hackers/2008-08/msg00102.php
Zdenek
--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql
Attachments:
gettemppage.patchtext/x-diff; name=gettemppage.patchDownload
diff -cr pgsql_temppage.7d94b24b3ce6/src/backend/access/gin/gindatapage.c pgsql_temppage/src/backend/access/gin/gindatapage.c
*** pgsql_temppage.7d94b24b3ce6/src/backend/access/gin/gindatapage.c út záŠ30 15:37:52 2008
--- pgsql_temppage/src/backend/access/gin/gindatapage.c út záŠ30 15:37:52 2008
***************
*** 445,451 ****
char *ptr;
OffsetNumber separator;
ItemPointer bound;
! Page lpage = GinPageGetCopyPage(BufferGetPage(lbuf));
ItemPointerData oldbound = *GinDataPageGetRightBound(lpage);
int sizeofitem = GinSizeOfItem(lpage);
OffsetNumber maxoff = GinPageGetOpaque(lpage)->maxoff;
--- 445,451 ----
char *ptr;
OffsetNumber separator;
ItemPointer bound;
! Page lpage = PageGetTempPage(BufferGetPage(lbuf), true);
ItemPointerData oldbound = *GinDataPageGetRightBound(lpage);
int sizeofitem = GinSizeOfItem(lpage);
OffsetNumber maxoff = GinPageGetOpaque(lpage)->maxoff;
diff -cr pgsql_temppage.7d94b24b3ce6/src/backend/access/gin/ginentrypage.c pgsql_temppage/src/backend/access/gin/ginentrypage.c
*** pgsql_temppage.7d94b24b3ce6/src/backend/access/gin/ginentrypage.c út záŠ30 15:37:52 2008
--- pgsql_temppage/src/backend/access/gin/ginentrypage.c út záŠ30 15:37:52 2008
***************
*** 458,464 ****
leftrightmost = NULL;
static ginxlogSplit data;
Page page;
! Page lpage = GinPageGetCopyPage(BufferGetPage(lbuf));
Page rpage = BufferGetPage(rbuf);
Size pageSize = PageGetPageSize(lpage);
--- 458,464 ----
leftrightmost = NULL;
static ginxlogSplit data;
Page page;
! Page lpage = PageGetTempPage(BufferGetPage(lbuf), true);
Page rpage = BufferGetPage(rbuf);
Size pageSize = PageGetPageSize(lpage);
diff -cr pgsql_temppage.7d94b24b3ce6/src/backend/access/gin/ginutil.c pgsql_temppage/src/backend/access/gin/ginutil.c
*** pgsql_temppage.7d94b24b3ce6/src/backend/access/gin/ginutil.c út záŠ30 15:37:52 2008
--- pgsql_temppage/src/backend/access/gin/ginutil.c út záŠ30 15:37:52 2008
***************
*** 309,329 ****
return entries;
}
- /*
- * It's analog of PageGetTempPage(), but copies whole page
- */
- Page
- GinPageGetCopyPage(Page page)
- {
- Size pageSize = PageGetPageSize(page);
- Page tmppage;
-
- tmppage = (Page) palloc(pageSize);
- memcpy(tmppage, page, pageSize);
-
- return tmppage;
- }
-
Datum
ginoptions(PG_FUNCTION_ARGS)
{
--- 309,314 ----
diff -cr pgsql_temppage.7d94b24b3ce6/src/backend/access/gin/ginvacuum.c pgsql_temppage/src/backend/access/gin/ginvacuum.c
*** pgsql_temppage.7d94b24b3ce6/src/backend/access/gin/ginvacuum.c út záŠ30 15:37:52 2008
--- pgsql_temppage/src/backend/access/gin/ginvacuum.c út záŠ30 15:37:52 2008
***************
*** 529,535 ****
* On first difference we create temporary page in memory
* and copies content in to it.
*/
! tmppage = GinPageGetCopyPage(origpage);
if (newN > 0)
{
--- 529,535 ----
* On first difference we create temporary page in memory
* and copies content in to it.
*/
! tmppage = PageGetTempPage(origpage, true);
if (newN > 0)
{
diff -cr pgsql_temppage.7d94b24b3ce6/src/backend/access/gist/gist.c pgsql_temppage/src/backend/access/gist/gist.c
*** pgsql_temppage.7d94b24b3ce6/src/backend/access/gist/gist.c út záŠ30 15:37:52 2008
--- pgsql_temppage/src/backend/access/gist/gist.c út záŠ30 15:37:52 2008
***************
*** 339,345 ****
* we must create temporary page to operate
*/
dist->buffer = state->stack->buffer;
! dist->page = PageGetTempPage(BufferGetPage(dist->buffer), sizeof(GISTPageOpaqueData));
/* clean all flags except F_LEAF */
GistPageGetOpaque(dist->page)->flags = (is_leaf) ? F_LEAF : 0;
--- 339,345 ----
* we must create temporary page to operate
*/
dist->buffer = state->stack->buffer;
! dist->page = PageGetTempPageCopySpecial(BufferGetPage(dist->buffer));
/* clean all flags except F_LEAF */
GistPageGetOpaque(dist->page)->flags = (is_leaf) ? F_LEAF : 0;
diff -cr pgsql_temppage.7d94b24b3ce6/src/backend/access/gist/gistvacuum.c pgsql_temppage/src/backend/access/gist/gistvacuum.c
*** pgsql_temppage.7d94b24b3ce6/src/backend/access/gist/gistvacuum.c út záŠ30 15:37:52 2008
--- pgsql_temppage/src/backend/access/gist/gistvacuum.c út záŠ30 15:37:52 2008
***************
*** 142,159 ****
UnlockReleaseBuffer(buffer);
}
- static Page
- GistPageGetCopyPage(Page page)
- {
- Size pageSize = PageGetPageSize(page);
- Page tmppage;
-
- tmppage = (Page) palloc(pageSize);
- memcpy(tmppage, page, pageSize);
-
- return tmppage;
- }
-
static ArrayTuple
vacuumSplitPage(GistVacuum *gv, Page tempPage, Buffer buffer, IndexTuple *addon, int curlenaddon)
{
--- 142,147 ----
***************
*** 322,328 ****
addon = (IndexTuple *) palloc(sizeof(IndexTuple) * lenaddon);
/* get copy of page to work */
! tempPage = GistPageGetCopyPage(page);
for (i = FirstOffsetNumber; i <= maxoff; i = OffsetNumberNext(i))
{
--- 310,316 ----
addon = (IndexTuple *) palloc(sizeof(IndexTuple) * lenaddon);
/* get copy of page to work */
! tempPage = PageGetTempPage(page, true);
for (i = FirstOffsetNumber; i <= maxoff; i = OffsetNumberNext(i))
{
diff -cr pgsql_temppage.7d94b24b3ce6/src/backend/access/nbtree/nbtinsert.c pgsql_temppage/src/backend/access/nbtree/nbtinsert.c
*** pgsql_temppage.7d94b24b3ce6/src/backend/access/nbtree/nbtinsert.c út záŠ30 15:37:52 2008
--- pgsql_temppage/src/backend/access/nbtree/nbtinsert.c út záŠ30 15:37:52 2008
***************
*** 793,799 ****
rbuf = _bt_getbuf(rel, P_NEW, BT_WRITE);
origpage = BufferGetPage(buf);
! leftpage = PageGetTempPage(origpage, sizeof(BTPageOpaqueData));
rightpage = BufferGetPage(rbuf);
_bt_pageinit(leftpage, BufferGetPageSize(buf));
--- 793,799 ----
rbuf = _bt_getbuf(rel, P_NEW, BT_WRITE);
origpage = BufferGetPage(buf);
! leftpage = PageGetTempPage(origpage, false);
rightpage = BufferGetPage(rbuf);
_bt_pageinit(leftpage, BufferGetPageSize(buf));
diff -cr pgsql_temppage.7d94b24b3ce6/src/backend/storage/page/bufpage.c pgsql_temppage/src/backend/storage/page/bufpage.c
*** pgsql_temppage.7d94b24b3ce6/src/backend/storage/page/bufpage.c út záŠ30 15:37:52 2008
--- pgsql_temppage/src/backend/storage/page/bufpage.c út záŠ30 15:37:52 2008
***************
*** 254,263 ****
/*
* PageGetTempPage
! * Get a temporary page in local memory for special processing
*/
Page
! PageGetTempPage(Page page, Size specialSize)
{
Size pageSize;
Page temp;
--- 254,264 ----
/*
* PageGetTempPage
! * Get a temporary page in local memory for special processing. Caller is
! * responsible to init page.
*/
Page
! PageGetTempPage(Page page, bool copy)
{
Size pageSize;
Page temp;
***************
*** 265,282 ****
pageSize = PageGetPageSize(page);
temp = (Page) palloc(pageSize);
- thdr = (PageHeader) temp;
! /* copy old page in */
! memcpy(temp, page, pageSize);
! /* set high, low water marks */
! thdr->pd_lower = SizeOfPageHeaderData;
! thdr->pd_upper = pageSize - MAXALIGN(specialSize);
! /* clear out the middle */
! MemSet((char *) temp + thdr->pd_lower, 0, thdr->pd_upper - thdr->pd_lower);
return temp;
}
--- 266,297 ----
pageSize = PageGetPageSize(page);
temp = (Page) palloc(pageSize);
! if( copy )
! {
! memcpy(temp, page, pageSize);
! }
! return temp;
! }
! /*
! * PageGetTempPageCopySpecial
! * Get a temporary page in local memory for special processing
! * XXX: only consumer now is gistplacetopage(), so maybe we don't even want it
! */
! Page
! PageGetTempPageCopySpecial(Page page)
! {
! Size pageSize;
! Page temp;
+ pageSize = PageGetPageSize(page);
+ temp = (Page) palloc(pageSize);
+
+ PageInit(temp, pageSize, PageGetSpecialSize(page));
+ memcpy(PageGetSpecialPointer(temp), PageGetSpecialPointer(page), PageGetSpecialSize(page));
+
return temp;
}
diff -cr pgsql_temppage.7d94b24b3ce6/src/include/access/gin.h pgsql_temppage/src/include/access/gin.h
*** pgsql_temppage.7d94b24b3ce6/src/include/access/gin.h út záŠ30 15:37:52 2008
--- pgsql_temppage/src/include/access/gin.h út záŠ30 15:37:52 2008
***************
*** 246,252 ****
extern Datum *extractEntriesS(GinState *ginstate, OffsetNumber attnum, Datum value,
int32 *nentries, bool *needUnique);
extern Datum *extractEntriesSU(GinState *ginstate, OffsetNumber attnum, Datum value, int32 *nentries);
- extern Page GinPageGetCopyPage(Page page);
extern Datum gin_index_getattr(GinState *ginstate, IndexTuple tuple);
extern OffsetNumber gintuple_get_attrnum(GinState *ginstate, IndexTuple tuple);
--- 246,251 ----
diff -cr pgsql_temppage.7d94b24b3ce6/src/include/storage/bufpage.h pgsql_temppage/src/include/storage/bufpage.h
*** pgsql_temppage.7d94b24b3ce6/src/include/storage/bufpage.h út záŠ30 15:37:52 2008
--- pgsql_temppage/src/include/storage/bufpage.h út záŠ30 15:37:52 2008
***************
*** 362,368 ****
extern bool PageHeaderIsValid(PageHeader page);
extern OffsetNumber PageAddItem(Page page, Item item, Size size,
OffsetNumber offsetNumber, bool overwrite, bool is_heap);
! extern Page PageGetTempPage(Page page, Size specialSize);
extern void PageRestoreTempPage(Page tempPage, Page oldPage);
extern void PageRepairFragmentation(Page page);
extern Size PageGetFreeSpace(Page page);
--- 362,369 ----
extern bool PageHeaderIsValid(PageHeader page);
extern OffsetNumber PageAddItem(Page page, Item item, Size size,
OffsetNumber offsetNumber, bool overwrite, bool is_heap);
! extern Page PageGetTempPage(Page page, bool copy);
! extern Page PageGetTempPageCopySpecial(Page page);
extern void PageRestoreTempPage(Page tempPage, Page oldPage);
extern void PageRepairFragmentation(Page page);
extern Size PageGetFreeSpace(Page page);
Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
I attach patch which cleans up code around PageGetTempPage. These changes were
discussed here:
http://archives.postgresql.org/pgsql-hackers/2008-08/msg00102.php
Applied with a minor change: instead of inventing
Page PageGetTempPage(Page page, bool copy)
I split it into two functions
Page PageGetTempPage(Page page)
Page PageGetTempPageCopy(Page page)
I don't see any advantage to having the single function, because it
doesn't seem like any calling code path would be likely to want both
behaviors depending on some condition. Moreover, the way you had it
meant that we'd be replacing
Page PageGetTempPage(Page page, Size specialSize);
with
Page PageGetTempPage(Page page, bool copy);
which seems risky to me. If someone failed to update code that was
meant to call the old API, they'd get no warning about it --- at least
not in any C compiler I'm familiar with. Changing the number of
arguments guarantees a compile error for un-updated code.
regards, tom lane
Tom Lane napsal(a):
Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
I attach patch which cleans up code around PageGetTempPage. These changes were
discussed here:
http://archives.postgresql.org/pgsql-hackers/2008-08/msg00102.phpApplied with a minor change: instead of inventing
Page PageGetTempPage(Page page, bool copy)
I split it into two functions
Page PageGetTempPage(Page page)
Page PageGetTempPageCopy(Page page)
I don't see any advantage to having the single function, because it
doesn't seem like any calling code path would be likely to want both
behaviors depending on some condition. Moreover, the way you had it
meant that we'd be replacing
Page PageGetTempPage(Page page, Size specialSize);
with
Page PageGetTempPage(Page page, bool copy);
which seems risky to me. If someone failed to update code that was
meant to call the old API, they'd get no warning about it --- at least
not in any C compiler I'm familiar with. Changing the number of
arguments guarantees a compile error for un-updated code.
Agree.
Just a question you sometime argue that somebody should uses this interface for
external module. Is there any clue which function is used and which not? Should
we create something like core API description which will be stable?
Zdenek
--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql