Re: Cleaning up nbtree after logical decoding on standby work

On Sun, May 28, 2023 at 7:49 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> IMO this makes sense for v16. These new arguments were introduced in
> v16, so if we have second thoughts, now is the right time to change
> them, before v16 is released. It will reduce the backpatching effort in
> the future; if we apply this in v17, then v16 will be the odd one out.

My current plan is to commit this later in the week, unless there are
further objections. Wednesday or possibly Thursday.

> Maybe add an assertion for that in _bt_search(), too. I know you added
> one in _bt_getroot(), and _bt_search() calls that as the very first
> thing. But I think it would be useful as documentation in _bt_search(), too.

Attached revision v3 does it that way.

> Maybe it would be more straightforward to always require heapRel in
> _bt_search() and _bt_getroot(), regardless of whether it's BT_READ or
> BT_WRITE, even if the functions don't use it with BT_READ. It would be
> less mental effort in the callers to just always pass in 'heapRel'.

Perhaps, but it would also necessitate keeping heapRel in
_bt_get_endpoint()'s signature. That would mean that
_bt_get_endpoint() would needlessly pass its own superfluous heapRel
arg to _bt_search(), while presumably never passing the same heapRel
to _bt_gettrueroot() (not to be confused with _bt_getroot) in the
"level == 0" case. These inconsistencies seem kind of jarring.

Besides, every _bt_search() caller must already understand that
_bt_search does non-obvious extra work for BT_WRITE callers -- that's
nothing new. The requirement that BT_WRITE callers pass a heapRel
exists precisely because code that is used only during BT_WRITE calls
might ultimately reach _bt_allocbuf() indirectly. The "no heapRel
required in BT_READ case" seems directly relevant to callers --
avoiding _bt_allocbuf() during _bt_search() calls during Hot Standby
(or during VACUUM) is a basic requirement that callers more or less
ask for and expect already. (Bear in mind that the new rule going
forward is that _bt_allocbuf() is the one and only choke point where
new pages/buffers can be allocated by nbtree, and the only possible
source of recovery conflicts during REDO besides opportunistic
deletion record conflicts -- so it really isn't strange for _bt_search
callers to be thinking about whether _bt_allocbuf is safe to call.)

--
Peter Geoghegan