Feedback-ID: id4a34324:Fastmail
Date: Mon, 1 Dec 2025 15:28:09 -0500
From: Andres Freund <andres@anarazel.de>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>,
	pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>, Noah Misch <noah@leadboat.com>,
	Robert Haas <robertmhaas@gmail.com>,
 Michael Paquier <michael.paquier@gmail.com>
Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
Message-ID: <js7t2fs6it2aljskgqzpqwj4uqc5geb4ztlcojsyh6bpzgaqew@nixo44lyx5vn>
References: <fvfmkr5kk4nyex56ejgxj3uzi63isfxovp2biecb4bspbjrze7@az2pljabhnff>
 <yivb2evcrj7fna5ymuunw3g5u5xxttwjbjxaa4ofkfkviystjv@4dfylftqxyxh>
 <6kmid26do57ykqfpvq6iieniy4djsymhrypkjccazq5g4bbe6a@2y6owwv7qpex>
 <CAEze2WgGe8vjj3jiWqUugWuwLJ9cLryaGrnASjm-yJ=tEALX2A@mail.gmail.com>
 <pmto7djq64mei53p7r5smfync2waittilhbuzc7j7lpflf2b3y@laz7r76y5pux>
 <CAEze2WjeK9CY003S4dmCugv_H4tz9AaXgnqW+wTc=BaPDg+2xg@mail.gmail.com>
 <3je3ahgf7rrmmurxo6hnlhg5d3ffwfrtjwjxd6jm5srlv5iebp@vxqk5qtgmowr>
 <3w7v3w6a57jnssokap4k7thoekig72flnyhd4wp3yftzdd7lm7@f6lpcfen6hr7>
 <6rgb2nvhyvnszz4ul3wfzlf5rheb2kkwrglthnna7qhe24onwr@vw27225tkyar>
 <CAAKRu_Y6H=_Jx=F77v_ArXza5wwDn-J1FiHr3_+F7aBBwy0TsQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAAKRu_Y6H=_Jx=F77v_ArXza5wwDn-J1FiHr3_+F7aBBwy0TsQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/js7t2fs6it2aljskgqzpqwj4uqc5geb4ztlcojsyh6bpzgaqew%40nixo44lyx5vn>
Precedence: bulk

Hi,

On 2025-11-21 12:52:38 -0500, Melanie Plageman wrote:
> > 0004: Use 64bit atomics for BufferDesc.state - at this point nothing uses the
> > additional bits yet, though.  Some annoying reformatting required to avoid
> > long lines.
>
> I noticed that the BUF_STATE_GET_REFCOUNT and BUF_STATE_GET_USAGECOUNT
> macros cast the return value to a uint32. We won't use the extra bits
> but we did bother to keep the macro result sized to the field width
> before so keeping it uint32 is probably more confusing now that state
> is 64 bit.

I can't really follow - why would we want to return a 64bit value if none of
the values ever can get anywhere near that big?


> Not related to this patch, but I noticed GetBufferDescriptor() calls
> for a uint32 and all the callers pretty much pass a signed int —
> wonder why it calls for uint32.

It's not strictly required - no Buffers exist bet INT32_MAX and
UINT32_MAX. However, GetBufferDescriptor() cannot be used for local buffers
(which would have a negative buffer id), therefore a uint32 is fine. Many of
the callers have dedicated branches to deal with local buffers and therefore
couldn't use a uint32.


> > 0006+0007: This is preparatory work for 0008, but also worthwhile on its
> > own. The private refcount stuff does show up in profiles. The reason it's
> > related is that without these changes the added information in 0008 makes that
> > worse.
>
> I found it slightly confusing that this commit appears to
> unnecessarily add the PrivateRefCountData struct (given that it
> doesn't need it to do the new parallel array thing). You could wait
> until you need it in 0008, but 0008 is big as it is, so it probably is
> fine where it is.

It seemed too annoying to whack the code around multiple times...


> in InitBufferManagerAccess(), why do you have
>
> memset(&PrivateRefCountArrayKeys, 0, sizeof(Buffer));
> seems like it should be
> memset(PrivateRefCountArrayKeys, 0, sizeof(PrivateRefCountArrayKeys));

Ugh, indeed.


> I wonder how easy it will be to keep the Buffer in sync between
> PrivateRefCountArrayKeys and the PrivateRefCountEntry — would a helper
> function help?

I don't think we really need it, the existing helper functions are where it
should be manipulated.


> ForgetPrivateRefCountEntry doesn’t clear the data member — but maybe
> it doesn’t matter...

It asserts that the fields are reset, which seems to suffice.


> in ReservePrivateRefCountEntry() there is a superfluous clear
>
> memset(&victim_entry->data, 0, sizeof(victim_entry->data));
> victim_entry->data.refcount = 0;

It's indeed superfluous today. I guess I put it in as a belt and suspenders
approach to future members... The compiler is easily be able to optimize that
redundancy away.


> 0007
> needs a commit message. overall seems fine though.

This is what I've since written:

    bufmgr: Add one-entry cache for private refcount

    The private refcount entry for a buffer is often looked up repeatedly for the
    same buffer, e.g. to pin and then unpin a buffer. Benchmarking shows that it's
    worthwhile to have a one-entry cache for that case. With that cache in place,
    it's worth splitting GetPrivateRefCountEntry() into a small inline
    portion (for the cache hit case) and an out-of-line helper for the rest.

    This is helpful for some workloads today, but becomes more important in an
    upcoming patch that will utilize the private refcount infrastructure to also
    store whether the buffer is currently locked, as that increases the rate of
    lookups substantially.


> You should probably capitalize the "c" of "count" in
> PrivateRefcountEntryLast to be consistent with the other names.

Ooops, yes.

Greetings,

Andres Freund