Обсуждение: ReadRecentBuffer() is broken for local buffer

ReadRecentBuffer() doesn't work for local buffers, i.e. for temp tables. 
The bug is pretty clear if you look at the code:

      if (BufferIsLocal(recent_buffer))
      {
-        bufHdr = GetBufferDescriptor(-recent_buffer - 1);
+        bufHdr = GetLocalBufferDescriptor(-recent_buffer - 1);

The code after that looks suspicious, too. It increases the usage count 
even if the buffer was already pinned. That's different from what it 
does for a shared buffer, and different from LocalBufferAlloc(). That's 
pretty harmless, just causes the usage count to be bumped more 
frequently, but I don't think it was intentional. The ordering of 
bumping the usage count, the local ref count, and registration in the 
resource owner are different too. As far as I can see, that makes no 
difference, but I think we should keep this code as close as possible to 
similar code used elsewhere, unless there's a particular reason to differ.

I propose the attached to fix those things.

I tested this by adding this little snippet to a random place where we 
have just read a page with ReadBuffer:

diff --git a/src/backend/access/heap/heapam.c 
b/src/backend/access/heap/heapam.c
index aab8d6fa4e5..c4abdbc96dd 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -403,6 +403,14 @@ heapgetpage(TableScanDesc sscan, BlockNumber page)
 
    RBM_NORMAL, scan->rs_strategy);
         scan->rs_cblock = page;

+       {
+               bool still_ok;
+
+               still_ok = 
ReadRecentBuffer(scan->rs_base.rs_rd->rd_locator, MAIN_FORKNUM, page, 
scan->rs_cbuf);
+               Assert(still_ok);
+               ReleaseBuffer(scan->rs_cbuf);
+       }
+
         if (!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE))
                 return;

Without the fix, the assertion is fails quickly on "make check".

- Heikki

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.

Nice catch, LGTM.



> On Jul 25, 2022, at 02:22, 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:
>
>     if (BufferIsLocal(recent_buffer))
>     {
> -        bufHdr = GetBufferDescriptor(-recent_buffer - 1);
> +        bufHdr = GetLocalBufferDescriptor(-recent_buffer - 1);
>
> The code after that looks suspicious, too. It increases the usage count even if the buffer was already pinned. That's
differentfrom what it does for a shared buffer, and different from LocalBufferAlloc(). That's pretty harmless, just
causesthe usage count to be bumped more frequently, but I don't think it was intentional. The ordering of bumping the
usagecount, the local ref count, and registration in the resource owner are different too. As far as I can see, that
makesno difference, but I think we should keep this code as close as possible to similar code used elsewhere, unless
there'sa particular reason to differ. 
>
> I propose the attached to fix those things.
>
> I tested this by adding this little snippet to a random place where we have just read a page with ReadBuffer:
>
> diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> index aab8d6fa4e5..c4abdbc96dd 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -403,6 +403,14 @@ heapgetpage(TableScanDesc sscan, BlockNumber page)
>   RBM_NORMAL, scan->rs_strategy);
>        scan->rs_cblock = page;
>
> +       {
> +               bool still_ok;
> +
> +               still_ok = ReadRecentBuffer(scan->rs_base.rs_rd->rd_locator, MAIN_FORKNUM, page, scan->rs_cbuf);
> +               Assert(still_ok);
> +               ReleaseBuffer(scan->rs_cbuf);
> +       }
> +
>        if (!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE))
>                return;
>
> Without the fix, the assertion is fails quickly on "make check".
>
> - Heikki<0001-Fix-ReadRecentBuffer-for-local-buffers.patch>

On 25/07/2022 00:35, Thomas Munro wrote:
> 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.

Pushed, thanks for the reviews.

- Heikki