On Sun, Sep 14, 2025 at 04:40:54PM +0300, Konstantin Knizhnik wrote:
> On 13/09/2025 10:10 PM, Peter Geoghegan wrote:
>> I think that _bt_split could easily be switched over to using 2 local
>> PGAlignedBlock variables, removing its use of PageGetTempPage. I don't
>> think that it is necessary to consider other PageGetTempPage callers.
Hmm. I didn't really agree with this last statement, especially if
these code paths don't need to manipulate Page pointers across the
stack. So I have taken this opportunity to double-check all the
existing calls of PageGetTempPage() which is an API old enough to vote
(105409746499?), while 44cac9346479 has introduced PGAlignedBlock much
later, seeing if there are some opportunities here:
- dataSplitPageInternal() and entrySplitPage() expect the contents of
the pages to be returned to the caller. We could use PGAlignedBlock
with pre-prepared pages that don't get allocated, but this requires
the callers of the internal routines to take responsibility for that,
like dataBeginPlaceToPage(). The complexity is not appealing in these
case.
- The same argument applies to the call in ginPlaceToPage(), passing
down the page to fillRoot().
- gistplacetopage(), with a copy of a page filled its special area,
manipulated across other calls.
- An optimization exists for btree_xlog_split() where a temporary page
is not manipulated, but the fact that we are in redo does not really
matter much for the extra allocation, I guess.
At the end, I don't feel excited about switching these cases, where
the gains are not obvious.
> Attached please find updated version of the patch which uses PGAlignedBlock
> instead of PageGetTempPage.
+ * high key. To prevent confision of VACUUM with orphan page, we also
+ * first fill in-mempory copy oif this page.
Three typos in two lines here (not sure about the best wording that
fits with VACUUM, but I like the suggested simplification):
s/in-mempory/in-memory/
s/confision/confusion/
s/oif/if/
Saying that, this patch sounds like a good idea, making these code
paths a bit more reliable for VACUUM, without paying the cost of an
allocation. At least for HEAD, that's nice to have. Peter, what do
you think?
--
Michael
