Re: Cleaning up nbtree after logical decoding on standby work

Hi,

On 2023-05-25 18:50:31 -0700, Peter Geoghegan wrote:
> Commit 61b313e4, which prepared index access method code for the
> logical decoding on standbys work, made quite a few mechanical
> changes. Many routines were revised in order to make sure that heaprel
> was available in _bt_getbuf()'s P_NEW (new page allocation) path. But
> this went further than it really had to. Many of the changes to nbtree
> seem excessive.
>
> We only need a heaprel in those code paths that might end up calling
> _bt_getbuf() with blkno = P_NEW. This includes most callers that pass
> access = BT_WRITE, and all callers that pass access = BT_READ. This
> doesn't have to be haphazard -- there just aren't that many places
> that can allocate new nbtree pages.

What do we gain by not passing down the heap relation to those places?
If you're concerned about the efficiency of passing down the parameters,
I doubt it will make a meaningful difference, because the parameter can
just stay in the register to be passed down further.

Note that I do agree with some aspects of the change for other reasons,
see below...

> It's just page splits, and new
> root page allocations (which are actually a slightly different kind of
> page split). The rule doesn't need to be complicated (to be fair it
> looks more complicated than it really is).
>
> Attached patch completely removes the changes to _bt_getbuf()'s
> signature from 61b313e4. This is possible without any loss of
> functionality. The patch splits _bt_getbuf () in two: the code that
> handles BT_READ/BT_WRITE stays in _bt_getbuf(), which is now far
> shorter. Handling of new page allocation is moved to a new routine
> I've called _bt_alloc(). This is clearer in general, and makes it
> clear what the rules really are. Any code that might need to call
> _bt_alloc() must be prepared for that, by having a heaprel to pass to
> it (the slightly complicated case is interrupted page splits).

I think it's a very good idea to split the "new page" case off
_bt_getbuf(). We probably should evolve the design in the area, and
that will be easier with such a change.

Greetings,

Andres Freund