Hi, On Tue, Jan 20, 2026 at 1:59 PM Chao Li <[email protected]> wrote: > > Good catch. But I think we should retain the explanation for -2. How about > like: > > For buf_internal.h: > ``` > /* > * Buffer's index number: >= 0 for shared buffers, < -1 for local > buffers. > * * For shared buffers, buf_id is 0 to NBuffers-1. > * * For local buffers, buf_id is -2 to -NLocBuffers-1. > * > * This ensures that (buf_id+1) done by BufferDescriptorGetBuffer > never returns InvalidBuffer(0). > */ > int buf_id; > ``` > > For localbuf.c > ``` > /* > * buf_id must be <= -2 for local buffers so that the > * Buffer handle (buf_id + 1) is <= -1, avoiding InvalidBuffer (0) > */ > ```
Thank you for looking into this! I agree with your suggestion to add "This ensures that ..." comment. But if we add it, I guess that we won't need to duplicate this comment in localbuf.c . BTW, maybe it is not good that we are hardcoding the name of the BufferDescriptorGetBuffer function in the comments, but I saw the same thing in several other places. Please, see the attached patch that includes your proposal. -- Best regards, Daniil Davydov
From 19e0f274486d5f81bf4fbdf26860e558756ede32 Mon Sep 17 00:00:00 2001 From: Daniil Davidov <[email protected]> Date: Tue, 20 Jan 2026 13:32:39 +0700 Subject: [PATCH v2] Fix comment in BufferDesc struct --- src/backend/storage/buffer/localbuf.c | 3 +-- src/include/storage/buf_internals.h | 8 ++++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c index 04a540379a2..ea67eca60c2 100644 --- a/src/backend/storage/buffer/localbuf.c +++ b/src/backend/storage/buffer/localbuf.c @@ -761,8 +761,7 @@ InitLocalBuffers(void) BufferDesc *buf = GetLocalBufferDescriptor(i); /* - * negative to indicate local buffer. This is tricky: shared buffers - * start with 0. We have to start with -2. (Note that the routine + * Negative to indicate local buffer. (Note that the routine * BufferDescriptorGetBuffer adds 1 to buf_id so our first buffer id * is -1.) */ diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 27f12502d19..0689e11e9d9 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -333,8 +333,12 @@ typedef struct BufferDesc BufferTag tag; /* - * Buffer's index number (from 0). The field never changes after - * initialization, so does not need locking. + * Buffer's index number : positive value (>= 0) for shared buffers and + * negative value (<= -2) for local buffers. The id ranges of both the + * local and shared buffers ensure that (buf_id + 1) executed by + * BufferDescriptorGetBuffer will never return InvalidBuffer(0). + * + * The field never changes after initialization, so does not need locking. */ int buf_id; -- 2.43.0
