Re: Cleaning up nbtree after logical decoding on standby work

On Fri, May 26, 2023 at 12:46 AM Michael Paquier <michael@paquier.xyz> wrote:
> Nice cleanup overall.

Thanks.

To be clear, when I said "it would be nice to get rid of P_NEW", what
I meant was that it would be nice to go much further than what I've
done in the patch by getting rid of the general idea of P_NEW. So the
handling of P_NEW at the top of ReadBuffer_common() would be removed,
for example. (Note that nbtree doesn't actually rely on that code,
even now; while its use of the P_NEW constant creates the impression
that it needs that bufmgr.c code, it actually doesn't, even now.)

> +    if (XLogStandbyInfoActive() && RelationNeedsWAL(rel))
>      {
> -        /* Okay to use page.  Initialize and return it. */
> -        _bt_pageinit(page, BufferGetPageSize(buf));
> -        return buf;
> +        safexid = BTPageGetDeleteXid(page);
> +        isCatalogRel = RelationIsAccessibleInLogicalDecoding(heaprel);
> +        _bt_log_reuse_page(rel, blkno, safexid, isCatalogRel);
>
> There is only one caller of _bt_log_reuse_page(), so assigning a
> boolean rather than the heap relation is a bit strange to me.  I think
> that calling RelationIsAccessibleInLogicalDecoding() within
> _bt_log_reuse_page() where xlrec_reuse is filled with its data is much
> more natural, like HEAD.

Attached is v2, which deals with this by moving the code from
_bt_log_reuse_page() into _bt_allocbuf() itself -- there is no need
for a separate logging function. This structure seems like a clear
improvement, since such logging is largely the point of having a
separate _bt_allocbuf() function that deals with new page allocations
and requires a valid heapRel in all cases.

v2 also renames "heaprel" to "heapRel" in function signatures, for
consistency with older code that always used that convention.

-- 
Peter Geoghegan