Thread: [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

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 inventingPage PageGetTempPage(Page page, bool copy)
I split it into two functionsPage 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 replacingPage PageGetTempPage(Page page, Size specialSize);
withPage 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.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.

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