Re: Reviewing freeze map code

On Sat, Jul 23, 2016 at 3:55 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Attached patch fixes the problem for me.  Note, I have not tried to
> reproduce the problem for heap_lock_updated_tuple_rec(), but I think
> if you are convinced with above cases, then we should have a similar
> check in it as well.

I don't think this hunk is correct:

+        /*
+         * If we didn't pin the visibility map page and the page has become
+         * all visible, we'll have to unlock and re-lock.  See heap_lock_tuple
+         * for details.
+         */
+        if (vmbuffer == InvalidBuffer && PageIsAllVisible(BufferGetPage(buf)))
+        {
+            LockBuffer(buf, BUFFER_LOCK_UNLOCK);
+            visibilitymap_pin(rel, block, &vmbuffer);
+            LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
+            goto l4;
+        }

The code beginning at label l4 appears that the buffer is unlocked,
but this code leaves the buffer unlocked.  Also, I don't see the point
of doing this test so far down in the function.  Why not just recheck
*immediately* after taking the buffer lock?  If you find out that you
need the pin after all, then           LockBuffer(buf,
BUFFER_LOCK_UNLOCK); visibilitymap_pin(rel, block, &vmbuffer);
LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE); but *do not* go back to l4.
Unless I'm missing something, putting this block further down, as you
have it, buys nothing, because none of that intervening code can
release the buffer lock without using goto to jump back to l4.

+    /*
+     * If we didn't pin the visibility map page and the page has become all
+     * visible while we were busy locking the buffer, or during some
+     * subsequent window during which we had it unlocked, we'll have to unlock
+     * and re-lock, to avoid holding the buffer lock across an I/O.  That's a
+     * bit unfortunate, especially since we'll now have to recheck whether the
+     * tuple has been locked or updated under us, but hopefully it won't
+     * happen very often.
+     */
+    if (vmbuffer == InvalidBuffer && PageIsAllVisible(page))
+    {
+        LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
+        visibilitymap_pin(relation, block, &vmbuffer);
+        LockBuffer(*buffer, BUFFER_LOCK_EXCLUSIVE);
+        goto l3;
+    }

In contrast, this looks correct: l3 expects the buffer to be locked
already, and the code above this point and below the point this logic
can unlock and re-lock the buffer, potentially multiple times.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company