postgres.git / summary / log / commit / refs

commit    fa7ff642c22ceccad869af5add00c2661d4d091e
Author:   Peter Geoghegan <pg@bowt.ie>
Date:     Sat Apr 25 21:17:02 2020 +0000

    Fix minor nbtree page deletion buffer lock issue.
    
    Avoid accessing the deletion target page's special area during nbtree
    page deletion at a point where there is no buffer lock held.  This issue
    was detected by a patch that extends Valgrind's memcheck tool to mark
    nbtree pages that are unsafe to access (due to not having a buffer lock
    or buffer pin) as NOACCESS.
    
    We do hold a buffer pin at this point, and only access the special area,
    so the old approach was safe.  Even still, it seems like a good idea to
    tighten up the rules in this area.  There is no reason to not simply
    insist on always holding a buffer lock (not just a pin) when accessing
    nbtree pages.
    
    Update some related comments in passing.
    
    Discussion: https://postgr.es/m/CAH2-WzkLgyN3zBvRZ1pkNJThC=xi_0gpWRUb_45eexLH1+k2_Q@mail.gmail.com


 src/backend/access/nbtree/nbtpage.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 39b8f17f4b5..321d887ff77 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -1611,7 +1611,7 @@ _bt_pagedel(Relation rel, Buffer buf)
 				 * to-be-deleted doesn't have a downlink, and the page
 				 * deletion algorithm isn't prepared to handle that.
 				 */
-				if (!P_LEFTMOST(opaque))
+				if (leftsib != P_NONE)
 				{
 					BTPageOpaque lopaque;
 					Page		lpage;
@@ -1695,6 +1695,12 @@ _bt_pagedel(Relation rel, Buffer buf)
 		 * the only child of the parent, and could be removed. It would be
 		 * picked up by the next vacuum anyway, but might as well try to
 		 * remove it now, so loop back to process the right sibling.
+		 *
+		 * Note: This relies on the assumption that _bt_getstackbuf() will be
+		 * able to reuse our original descent stack with a different child
+		 * block (provided that the child block is to the right of the
+		 * original leaf page reached by _bt_search()). It will even update
+		 * the descent stack each time we loop around, avoiding repeated work.
 		 */
 		if (!rightsib_empty)
 			break;
@@ -1904,8 +1910,8 @@ _bt_mark_page_halfdead(Relation rel, Buffer leafbuf, BTStack stack)
  * leaf page is deleted.
  *
  * Returns 'false' if the page could not be unlinked (shouldn't happen).
- * If the (new) right sibling of the page is empty, *rightsib_empty is set
- * to true.
+ * If the (current) right sibling of the page is empty, *rightsib_empty is
+ * set to true.
  *
  * Must hold pin and lock on leafbuf at entry (read or write doesn't matter).
  * On success exit, we'll be holding pin and write lock.  On failure exit,


[parent: f246ea3b2a5e]