Обсуждение: Removing unneeded downlink field from nbtree stack struct

Attached patch slightly simplifies _bt_getstackbuf() by making it
accept a child BlockNumber argument, rather than requiring that
callers store the child block number in the parent stack item's
bts_btentry field. We can remove the bts_btentry field from the
BTStackData struct, because we know where we ended up when we split a
page and need to relocate parent to insert new downlink -- it's only
truly necessary to remember what pivot tuple/downlink we followed to
arrive at the page being split. There is no point in remembering the
child block number during our initial descent of a B-Tree, since it's
never actually used at a later point, and can go stale immediately
after the buffer lock on parent is released. Besides,
_bt_getstackbuf() callers can even redefine the definition of child to
be child's right sibling after the descent is over. For example, this
happens when we move right, or when we step right during unique index
insertion.

This slightly simplifies the code. Our stack is inherently
approximate, because we might have to move right for a number of
reasons.

I'll add the patch to the 2019-09 CF.
-- 
Peter Geoghegan

16.07.2019 2:16, Peter Geoghegan wrote:
> Attached patch slightly simplifies _bt_getstackbuf() by making it
> accept a child BlockNumber argument, rather than requiring that
> callers store the child block number in the parent stack item's
> bts_btentry field. We can remove the bts_btentry field from the
> BTStackData struct, because we know where we ended up when we split a
> page and need to relocate parent to insert new downlink -- it's only
> truly necessary to remember what pivot tuple/downlink we followed to
> arrive at the page being split. There is no point in remembering the
> child block number during our initial descent of a B-Tree, since it's
> never actually used at a later point, and can go stale immediately
> after the buffer lock on parent is released. Besides,
> _bt_getstackbuf() callers can even redefine the definition of child to
> be child's right sibling after the descent is over. For example, this
> happens when we move right, or when we step right during unique index
> insertion.
>
> This slightly simplifies the code. Our stack is inherently
> approximate, because we might have to move right for a number of
> reasons.
>
> I'll add the patch to the 2019-09 CF.


The refactoring is clear, so I set Ready for committer status.
I have just a couple of notes about comments:

1) I think that it's worth to add explanation of the case when we use 
right sibling to this comment:
+                * stack to work back up to the parent page.  We use the 
child block
+                * number (or possibly the block number of a page to its 
right)

2) It took me quite some time to understand why does page deletion case 
doesn't need a lock.
I propose to add something like "For more see comments for 
_bt_lock_branch_parent()" to this line:

Page deletion caller
+ *             can get away with a lock on leaf level page when 
locating topparent
+ *             downlink, though.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Aug 12, 2019 at 9:43 AM Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:
> The refactoring is clear, so I set Ready for committer status.
> I have just a couple of notes about comments:
>
> 1) I think that it's worth to add explanation of the case when we use
> right sibling to this comment:
> +                * stack to work back up to the parent page.  We use the
> child block
> +                * number (or possibly the block number of a page to its
> right)

That appears over _bt_getstackbuf().

> 2) It took me quite some time to understand why does page deletion case
> doesn't need a lock.
> I propose to add something like "For more see comments for
> _bt_lock_branch_parent()" to this line:

I ended up removing the reference to page deletion here (actually, I
removed the general discussion about the need to keep the child page
locked). This seemed like something that was really up to the callers.

Pushed a version with that change. Thanks for the review!

-- 
Peter Geoghegan