Thread: unnecessary code in_bt_split

I found that _bt_split function calls PageGetTempPage, but next call is 
_bt_page_init which clear all contents anyway. Is there any reason to call 
PageGetTempPage instead of palloc?

Original code:

00796     leftpage = PageGetTempPage(origpage, sizeof(BTPageOpaqueData));
00797     rightpage = BufferGetPage(rbuf);
00798
00799     _bt_pageinit(leftpage, BufferGetPageSize(buf));

Suggested code:

00796     leftpage = palloc(PageGetSize(origpage));
00797     rightpage = BufferGetPage(rbuf);
00798
00799     _bt_pageinit(leftpage, BufferGetPageSize(buf));

Any idea?
    thanks Zdenek



-- 
Zdenek Kotala              Sun Microsystems
Prague, Czech Republic     http://sun.com/postgresql

Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> I found that _bt_split function calls PageGetTempPage, but next call is 
> _bt_page_init which clear all contents anyway. Is there any reason to call 
> PageGetTempPage instead of palloc?

Not violating a perfectly good abstraction?

I agree that PageGetTempPage isn't amazingly efficient, but internal
refactoring would halve its cost; and if you have some evidence that
there's a real performance issue then we could think about adjusting
the temp-page API to allow _bt_pageinit to be combined with it.  But
I have a real problem with hacking up _bt_split so that it will call
PageRestoreTempPage on something it didn't get from PageGetTempPage.

Considering the WAL and regular I/O that will be induced by a split,
I kinda doubt this is even worth worrying about anyway...
        regards, tom lane

On Sun, 2008-08-03 at 19:44 -0400, Tom Lane wrote:
> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> > I found that _bt_split function calls PageGetTempPage, but next call is 
> > _bt_page_init which clear all contents anyway. Is there any reason to call 
> > PageGetTempPage instead of palloc?
> 
> Not violating a perfectly good abstraction?
> 
> I agree that PageGetTempPage isn't amazingly efficient, but internal
> refactoring would halve its cost; and if you have some evidence that
> there's a real performance issue then we could think about adjusting
> the temp-page API to allow _bt_pageinit to be combined with it.  But
> I have a real problem with hacking up _bt_split so that it will call
> PageRestoreTempPage on something it didn't get from PageGetTempPage.
> 
> Considering the WAL and regular I/O that will be induced by a split,
> I kinda doubt this is even worth worrying about anyway...

Improving this should help, since the existing page is write locked
during _bt_split. The I/O won't happen at the point that these blocks
are critical contention points.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Tom Lane napsal(a):
> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>> I found that _bt_split function calls PageGetTempPage, but next call is 
>> _bt_page_init which clear all contents anyway. Is there any reason to call 
>> PageGetTempPage instead of palloc?
> 
> Not violating a perfectly good abstraction?

OK. Abstraction is nice, but what I see in the PageGetTempPage It is more like 
code which makes everything but usability is zero. It is used only in two places 
and in both it is used for different purpose. _bt_split() needs only allocate 
empty temp page and gistplacetopage() .

 By my opinion It would be better to have three functions:

PageCreateTempPage - only allocate memory and call pageinit
PageCloneSpecial - copy special section from source page
PageRestoreTempPage - no change.

    Zdenek

Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> Tom Lane napsal(a):
>> Not violating a perfectly good abstraction?

>   By my opinion It would be better to have three functions:

> PageCreateTempPage - only allocate memory and call pageinit
> PageCloneSpecial - copy special section from source page
> PageRestoreTempPage - no change.

That naming still breaks the association of "TempPage" functions.
If we're going to have multiple temp-page-creation functions,
I think their names should follow a pattern like PageGetTempPageXXX.

After looking around a bit, I'm not entirely convinced that there's
*any* call for the existing definition of PageGetTempPage :-(.
There are only two callers: _bt_split() which certainly doesn't need
it to work the way it does, and gistplacetopage() which might or
might not be just as happy initializing all of the page special
space for itself.  Oleg, Teodor, could you comment on whether it's
really needed to copy the old page's special space there?

Also, to the extent that PageGetTempPage copies the source page's
header instead of setting it up from scratch, I think it's outright
*wrong*.  This will result in copying the source's pd_flags and
pd_prune_xid, neither of which seems like correct behavior given that
we're clearing the page contents.

I'm thinking we should split PageGetTempPage into two versions:

PageGetTempPage: get a temp page the same size as the given page,
but don't initialize its contents at all (so, just a thin wrapper
for palloc).  This could be used by _bt_split, as well as
GinPageGetCopyPage and GistPageGetCopyPage.

PageGetTempPageCopySpecial: get a temp page, PageInit it, and
copy the special space from the given page.  The only customer
for this is gistplacetopage(), so maybe we don't even want it,
rather than just doing the work right in gistplacetopage()?

You could also make an argument for PageGetTempPageCopy() which'd just
copy the source page verbatim, thus replacing GinPageGetCopyPage and
GistPageGetCopyPage.
        regards, tom lane

Tom Lane napsal(a):
> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:

> I'm thinking we should split PageGetTempPage into two versions:
> 
> PageGetTempPage: get a temp page the same size as the given page,
> but don't initialize its contents at all (so, just a thin wrapper
> for palloc).  This could be used by _bt_split, as well as
> GinPageGetCopyPage and GistPageGetCopyPage.
> 
> PageGetTempPageCopySpecial: get a temp page, PageInit it, and
> copy the special space from the given page.  The only customer
> for this is gistplacetopage(), so maybe we don't even want it,
> rather than just doing the work right in gistplacetopage()?
> 
> You could also make an argument for PageGetTempPageCopy() which'd just
> copy the source page verbatim, thus replacing GinPageGetCopyPage and
> GistPageGetCopyPage.

Sounds good I will look on it.
    Zdenek


-- 
Zdenek Kotala              Sun Microsystems
Prague, Czech Republic     http://sun.com/postgresql