Re: Removing unneeded downlink field from nbtree stack struct

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