On 25/09/2025 7:35 AM, Michael Paquier wrote:
> On Mon, Sep 22, 2025 at 07:44:18PM +0300, Константин Книжник wrote:
>> Attached please find rebased version of the patch with fixed mistypings.
> I have looked at v3.
>
> - leftpage = PageGetTempPage(origpage);
> + leftpage = leftpage_buf.data;
> + memcpy(leftpage, origpage, BLCKSZ);
> _bt_pageinit(leftpage, BufferGetPageSize(buf));
>
> What's the point of copying the contents of origpage to leftpage?
> _bt_pageinit() is going to initialize leftpage (plus a few more things
> set like the page LSN, etc.), so the memcpy should not be necessary,
> no?
Sorry, memcpy is really not needed here.
>
> + rightpage = BufferGetPage(rbuf);
> + memcpy(rightpage, rightpage_buf.data, BLCKSZ);
> + ropaque = BTPageGetOpaque(rightpage);
>
> When we reach this state of the logic, aka at the beginning of the
> critical section, the right and left pages are in a correct state,
> and your code is OK because we copy the contents of the right page
> back into its legitimate place. What is not OK to me is that the
> copy of the "temporary" right page back to "rbuf" is not explained. I
> think that this deserves a comment, especially the part about ropaque
> which is set once by this proposal, then manipulated while in the
> critical section.
I added the comment, please check that it is clear and complete.
Updated version of the patch is attached.
