diff options
| author | Peter Geoghegan <pg@bowt.ie> | 2020-04-25 14:17:02 -0700 |
|---|---|---|
| committer | Peter Geoghegan <pg@bowt.ie> | 2020-04-25 14:17:02 -0700 |
| commit | fa7ff642c22ceccad869af5add00c2661d4d091e (patch) | |
| tree | a797b4c7b0818c64c5e1143f6e222beaf2c74fa7 /src | |
| parent | f246ea3b2a5e0b75e44f0f18157c4b5e10b5547f (diff) | |
| download | postgresql-fa7ff642c22ceccad869af5add00c2661d4d091e.tar.gz postgresql-fa7ff642c22ceccad869af5add00c2661d4d091e.zip | |
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
Diffstat (limited to 'src')
| -rw-r--r-- | src/backend/access/nbtree/nbtpage.c | 12 |
1 files 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, |