Re: ReadRecentBuffer() is broken for local buffer

On Mon, Jul 25, 2022 at 6:22 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> ReadRecentBuffer() doesn't work for local buffers, i.e. for temp tables.
> The bug is pretty clear if you look at the code:

-        bufHdr = GetBufferDescriptor(-recent_buffer - 1);
+        int            b = -recent_buffer - 1;
+
+        bufHdr = GetLocalBufferDescriptor(b);

Ugh, right.  Obviously this code path is not reached currently.  I
added the local path for completeness but I didn't think of the idea
of testing it the way you suggested, hence thinko escaped into the
wild.  That way of testing seems good and the patch indeed fixes the
problem.

-            /* Bump local buffer's ref and usage counts. */
+            /*
+             * Bump buffer's ref and usage counts. This is equivalent of
+             * PinBuffer for a shared buffer.
+             */
+            if (LocalRefCount[b] == 0)
+            {
+                if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)
+                {
+                    buf_state += BUF_USAGECOUNT_ONE;
+                    pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
+                }
+            }
+            LocalRefCount[b]++;
             ResourceOwnerRememberBuffer(CurrentResourceOwner, recent_buffer);
-            LocalRefCount[-recent_buffer - 1]++;
-            if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)
-                pg_atomic_write_u32(&bufHdr->state,
-                                    buf_state + BUF_USAGECOUNT_ONE);

+1, it makes sense to do it only if it wasn't pinned already, and it
really should look identical to the code in LocalBufferAlloc, and
perhaps the comment should even say so.

LGTM.