On 29/05/2023 03:31, Michael Paquier wrote:
> On Fri, May 26, 2023 at 04:48:37PM -0700, Peter Geoghegan wrote:
>> I'd have thought the subject line "Cleaning up nbtree after logical
>> decoding on standby work" made it quite clear that this patch was
>> targeting 16.
>
> Hmm, okay. I was understanding that as something for v17, honestly.
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.
For the patch itself:
> @@ -75,6 +74,10
> * _bt_search() -- Search the tree for a particular scankey,
> * or more precisely for the first leaf page it could be on.
> *
> + * rel must always be provided. heaprel must be provided by callers that pass
> + * access = BT_WRITE, since we may need to allocate a new root page for caller
> + * in passing (or when finishing a page split results in a parent page split).
> + *
> * The passed scankey is an insertion-type scankey (see nbtree/README),
> * but it can omit the rightmost column(s) of the index.
> *
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.
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'.
Overall, +1 on this patch, and +1 for committing it to v16.
--
Heikki Linnakangas
Neon (https://neon.tech)