On 2016-07-18 10:02:52 +0530, Amit Kapila wrote: > On Mon, Jul 18, 2016 at 9:13 AM, Andres Freund <and...@anarazel.de> wrote: > > On 2016-07-18 09:07:19 +0530, Amit Kapila wrote: > >> + /* > >> + * Before locking the buffer, pin the visibility map page if it may be > >> + * necessary. > >> + */ > >> > >> + if (PageIsAllVisible(BufferGetPage(*buffer))) > >> + visibilitymap_pin(relation, block, &vmbuffer); > >> + > >> LockBuffer(*buffer, BUFFER_LOCK_EXCLUSIVE); > >> > >> I think we need to check for PageIsAllVisible and try to pin the > >> visibility map after taking the lock on buffer. I think it is quite > >> possible that in the time this routine tries to acquire lock on > >> buffer, the page becomes all visible. > > > > I don't see how. Without a cleanup lock it's not possible to mark a page > > all-visible/frozen. > > > > Consider the below scenario. > > Vacuum > a. acquires a cleanup lock for page - 10 > b. busy in checking visibility of tuples > --assume, here it takes some time and in the meantime Session-1 > performs step (a) and (b) and start waiting in step- (c) > c. marks the page as all-visible (PageSetAllVisible) > d. unlockandrelease the buffer > > Session-1 > a. In heap_lock_tuple(), readbuffer for page-10 > b. check PageIsAllVisible(), found page is not all-visible, so didn't > acquire the visbilitymap_pin > c. LockBuffer in ExlusiveMode - here it will wait for vacuum to > release the lock > d. Got the lock, but now the page is marked as all-visible, so ideally > need to recheck the page and acquire the visibilitymap_pin
So, I've tried pretty hard to reproduce that. While the theory above is sound, I believe the relevant code-path is essentially dead for SQL callable code, because we'll always hold a buffer pin before even entering heap_update/heap_lock_tuple. It's possible that you could concoct a dangerous scenario with follow_updates though; but I can't immediately see how. Due to that, and based on the closing in beta release, I'm planning to push a version of the patch that the returns fixed; but not this. It seems better to have the majority of the fix in. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
