Re: [HACKERS] unnecessary code in_bt_split
Tom Lane napsal(a): Zdenek Kotala [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unnecessary code in_bt_split
On Sun, 2008-08-03 at 19:44 -0400, Tom Lane wrote: Zdenek Kotala [EMAIL PROTECTED] 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.com PostgreSQL Training, Services and Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unnecessary code in_bt_split
Tom Lane napsal(a): Zdenek Kotala [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unnecessary code in_bt_split
Zdenek Kotala [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unnecessary code in_bt_split
Zdenek Kotala [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers