MIME-Version: 1.0
References: <fmkqmyeyy7bdpvcgkheb6yaqewemkik3ls6aaveyi5ibmvtxnd@nu2kvy5rq3a6>
 <CAAKRu_a0w7wVZCysJ1s6tEPkdsGyYR4iBSEUefa3Y2nWLx2vSw@mail.gmail.com>
 <tsoi4ahsvxl7ln3ltmkhi4wn6p5b36uiokll5j2mzsfhpl6i7w@brc33vsxcdum>
In-Reply-To: <tsoi4ahsvxl7ln3ltmkhi4wn6p5b36uiokll5j2mzsfhpl6i7w@brc33vsxcdum>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Sat, 7 Mar 2026 12:33:05 -0500
Message-ID: 
 <CAAKRu_b8+MYK1JsppdtUNV-f3UgEg3F7YihiSWvxoHeWgEN7BA@mail.gmail.com>
Subject: Re: Unlogged rel fake lsn vs GetVictimBuffer()
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org, Heikki Linnakangas <hlinnaka@iki.fi>,
	Peter Geoghegan <pg@bowt.ie>
Content-Type: multipart/mixed; boundary="000000000000a31c31064c72904d"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAKRu_b8%2BMYK1JsppdtUNV-f3UgEg3F7YihiSWvxoHeWgEN7BA%40mail.gmail.com>
Precedence: bulk

--000000000000a31c31064c72904d
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Feb 28, 2026 at 7:16=E2=80=AFPM Andres Freund <andres@anarazel.de> =
wrote:
>
> > +BufferNeedsWALFlush(BufferDesc *bufdesc, bool exclusive_locked)
> > +{
> > +     uint32          buf_state =3D pg_atomic_read_u64(&bufdesc->state)=
;
> > +     Buffer          buffer;
> > +     char       *page;
> > +     XLogRecPtr      lsn;
> > +
> > +     /*
> > +      * Unlogged buffers can't need WAL flush. See FlushBuffer() for m=
ore
> > +      * details on unlogged relations with LSNs.
> > +      */
> > +     if (!(buf_state & BM_PERMANENT))
> > +             return false;
> > +
> > +     buffer =3D BufferDescriptorGetBuffer(bufdesc);
> > +     page =3D BufferGetPage(buffer);
> > +
> > +     if (!XLogHintBitIsNeeded() || BufferIsLocal(buffer) || exclusive_=
locked)
> > +             lsn =3D PageGetLSN(page);
> > +     else
> > +     {
> > +             /* Buffer is either share locked or not locked */
> > +             LockBufHdr(bufdesc);
> > +             lsn =3D PageGetLSN(page);
> > +             UnlockBufHdr(bufdesc);
> > +     }
>
> I buy the exclusive_locked branch, but the rest seems a bit dubious:
>
> - BufferIsLocal(buffer) is impossible, the buffer would not be permanent,=
 and
>   I don't think we use strategy

Changed it to an assert.

> - XLogHintBitIsNeeded() isn't *that* cheap and if we dirtied the buffer, =
the
>   relative cost of locking the buffer header isn't going to matter.

BufferGetLSNAtomic() checks XLogHintBitIsNeeded() as part of what it
calls a "fast path". I suppose in the case of BufferNeedsWALFlush() we
expect it only to be called on dirty buffers, so perhaps we don't need
to worry about being "fast". But why would XLogHintBitIsNeeded() be
slower than the buffer header lock? Is accessing the ControlFile
variable expensive or is it just the external function call?

> > diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storag=
e/buffer/freelist.c
>
> > @@ -795,8 +800,14 @@ StrategyRejectBuffer(BufferAccessStrategy strategy=
, BufferDesc *buf, bool from_r
> >               strategy->buffers[strategy->current] !=3D BufferDescripto=
rGetBuffer(buf))
> >               return false;
> >
> > +     Assert(BufferIsLockedByMe(BufferDescriptorGetBuffer(buf)));
> > +     Assert(!(pg_atomic_read_u64(&buf->state) & BM_LOCKED));
> > +
> > +     if (!BufferNeedsWALFlush(buf, false))
> > +             return false;
>
> It's a bit unfortunate that now you have an external function call from
> bufmgr.c (for StrategyRejectBuffer()) and then an external call back to
> bufmgr.c, just to then return back to freelist.c and then to bufmgr.c.  A=
nd
> you need to re-read the buf_state in BufferNeedsWALFlush() again.
>
> I'd probably just call BufferNeedsWALFlush() in GetVictimBuffer() and pas=
s it
> the old buf_state, to avoid having to re-fetch it.

My idea was to avoid taking the buffer header lock if it is a
non-bulkread strategy -- which you can't do if you need to call
BufferNeedsWALFlush() from bufmgr.c. You have to check
BufferNeedsWALFlush() before StrategyRejectBuffer() because
StrategyRejectBuffer() sets the buffer to InvalidBuffer in the ring.
So you think that the external function call will be more expensive
than taking the buffer header lock?

And, I find the current StrategyRejectBuffer() kind of silly and hard
to understand -- its comment mentions needing WAL flush, but, of
course it doesn't check that at all. I've changed it as you suggest,
but I did really prefer it the other way from a code readability
perspective.

- Melanie

--000000000000a31c31064c72904d
Content-Type: text/x-patch; charset="US-ASCII";
	name="v2-0001-Avoid-WAL-flush-checks-for-unlogged-buffers-in-Ge.patch"
Content-Disposition: attachment;
	filename="v2-0001-Avoid-WAL-flush-checks-for-unlogged-buffers-in-Ge.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_mmglq3ia0>
X-Attachment-Id: f_mmglq3ia0

RnJvbSA3ZGI3YWE2OWY1YzU2YWEyYWU0MTlkOTliMzMwOGE2MjA5ZjZmODJhIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBNZWxhbmllIFBsYWdlbWFuIDxtZWxhbmllcGxhZ2VtYW5AZ21h
aWwuY29tPgpEYXRlOiBTYXQsIDcgTWFyIDIwMjYgMTE6Mzg6MTQgLTA1MDAKU3ViamVjdDogW1BB
VENIIHYyXSBBdm9pZCBXQUwtZmx1c2ggY2hlY2tzIGZvciB1bmxvZ2dlZCBidWZmZXJzIGluCiBH
ZXRWaWN0aW1CdWZmZXIoKQoKR2V0VmljdGltQnVmZmVyKCkgcmVqZWN0cyBhIHZpY3RpbSBidWZm
ZXIgaWYgaXQgaXMgZnJvbSBhIGJ1bGtyZWFkCnN0cmF0ZWd5IHJpbmcgYW5kIHJldXNpbmcgaXQg
d291bGQgcmVxdWlyZSBmbHVzaGluZyBXQUwuIFVubG9nZ2VkIHRhYmxlCmJ1ZmZlcnMgY2FuIGhh
dmUgZmFrZSBMU05zIChlLmcuIHVubG9nZ2VkIEdpU1QgcGFnZXMpIGFuZCBjYWxsaW5nClhMb2dO
ZWVkc0ZsdXNoKCkgb24gYSBmYWtlIExTTiBpcyBtZWFuaW5nbGVzcy4KClRoaXMgaXNuJ3QgYW4g
aXNzdWUgY3VycmVudGx5IGJlY2F1c2UgdGhlIGJ1bGtyZWFkIHN0cmF0ZWd5IGlzIG5vdCB1c2Vk
CmZvciByZWxhdGlvbnMgd2l0aCBmYWtlIExTTnMuIEhvd2V2ZXIsIGZpeGluZyBpdCBhdm9pZHMg
ZnV0dXJlIGlzc3VlcwphbmQgYXZvaWRzIGFjcXVpcmluZyB0aGUgYnVmZmVyIGhlYWRlciBsb2Nr
IGZvciB1bmxvZ2dlZCB0YWJsZSBidWZmZXJzLgpXaGlsZSB3ZSdyZSBhdCBpdCwgYXZvaWQgdGFr
aW5nIHRoZSBidWZmZXIgaGVhZGVyIGxvY2sgZm9yIGJ1ZmZlcnMgbm90CmZyb20gdGhlIHN0cmF0
ZWd5IHJpbmcuCgpBdXRob3I6IE1lbGFuaWUgUGxhZ2VtYW4gPG1lbGFuaWVwbGFnZW1hbkBnbWFp
bC5jb20+ClJlcG9ydGVkLWJ5OiBBbmRyZXMgRnJldW5kIDxhbmRyZXNAYW5hcmF6ZWwuZGU+ClJl
dmlld2VkLWJ5OiBBbmRyZXMgRnJldW5kIDxhbmRyZXNAYW5hcmF6ZWwuZGU+ClJldmlld2VkLWJ5
OiBDaGFvIExpIDxsaS5ldmFuLmNoYW9AZ21haWwuY29tPgpEaXNjdXNzaW9uOiBodHRwczovL3Bv
c3Rnci5lcy9tL2ZsYXQvZm1rcW15ZXl5N2JkcHZjZ2toZWI2eWFxZXdlbWtpazNsczZhYXZleWk1
aWJtdnR4bmQlNDBudTJrdnk1cnEzYTYKLS0tCiBzcmMvYmFja2VuZC9hY2Nlc3MvdHJhbnNhbS94
bG9nLmMgICAgIHwgIDMgKysKIHNyYy9iYWNrZW5kL3N0b3JhZ2UvYnVmZmVyL2J1Zm1nci5jICAg
fCA2OCArKysrKysrKysrKysrKysrKysrKystLS0tLS0KIHNyYy9iYWNrZW5kL3N0b3JhZ2UvYnVm
ZmVyL2ZyZWVsaXN0LmMgfCAgNCArLQogMyBmaWxlcyBjaGFuZ2VkLCA1OSBpbnNlcnRpb25zKCsp
LCAxNiBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC9hY2Nlc3MvdHJhbnNh
bS94bG9nLmMgYi9zcmMvYmFja2VuZC9hY2Nlc3MvdHJhbnNhbS94bG9nLmMKaW5kZXggYjliNjc4
ZjM3MjIuLmY4NDZmMzgxNGI0IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9hY2Nlc3MvdHJhbnNh
bS94bG9nLmMKKysrIGIvc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0veGxvZy5jCkBAIC0zMTgy
LDYgKzMxODIsOSBAQCBYTG9nTmVlZHNGbHVzaChYTG9nUmVjUHRyIHJlY29yZCkKIAkJCXJldHVy
biB0cnVlOwogCX0KIAorCS8qIFByb3RlY3QgYWdhaW5zdCBjYWxsZXJzIHVzaW5nIGZha2UgTFNO
cyAqLworCUFzc2VydChyZWNvcmQgPD0gR2V0WExvZ0luc2VydFJlY1B0cigpKTsKKwogCS8qIFF1
aWNrIGV4aXQgaWYgYWxyZWFkeSBrbm93biBmbHVzaGVkICovCiAJaWYgKHJlY29yZCA8PSBMb2d3
cnRSZXN1bHQuRmx1c2gpCiAJCXJldHVybiBmYWxzZTsKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5k
L3N0b3JhZ2UvYnVmZmVyL2J1Zm1nci5jIGIvc3JjL2JhY2tlbmQvc3RvcmFnZS9idWZmZXIvYnVm
bWdyLmMKaW5kZXggNWYzZDA4M2U5MzguLjZkMTZmODcyNjc0IDEwMDY0NAotLS0gYS9zcmMvYmFj
a2VuZC9zdG9yYWdlL2J1ZmZlci9idWZtZ3IuYworKysgYi9zcmMvYmFja2VuZC9zdG9yYWdlL2J1
ZmZlci9idWZtZ3IuYwpAQCAtNjI1LDYgKzYyNSw4IEBAIHN0YXRpYyBib29sIFBpbkJ1ZmZlcihC
dWZmZXJEZXNjICpidWYsIEJ1ZmZlckFjY2Vzc1N0cmF0ZWd5IHN0cmF0ZWd5LAogc3RhdGljIHZv
aWQgUGluQnVmZmVyX0xvY2tlZChCdWZmZXJEZXNjICpidWYpOwogc3RhdGljIHZvaWQgVW5waW5C
dWZmZXIoQnVmZmVyRGVzYyAqYnVmKTsKIHN0YXRpYyB2b2lkIFVucGluQnVmZmVyTm9Pd25lcihC
dWZmZXJEZXNjICpidWYpOworc3RhdGljIGJvb2wgQnVmZmVyTmVlZHNXQUxGbHVzaChCdWZmZXJE
ZXNjICpidWZkZXNjLCB1aW50NjQgYnVmX3N0YXRlLAorCQkJCQkJCQlib29sIGV4Y2x1c2l2ZV9s
b2NrZWQpOwogc3RhdGljIHZvaWQgQnVmZmVyU3luYyhpbnQgZmxhZ3MpOwogc3RhdGljIGludAlT
eW5jT25lQnVmZmVyKGludCBidWZfaWQsIGJvb2wgc2tpcF9yZWNlbnRseV91c2VkLAogCQkJCQkJ
ICBXcml0ZWJhY2tDb250ZXh0ICp3Yl9jb250ZXh0KTsKQEAgLTI1MjIsMjIgKzI1MjQsMTMgQEAg
YWdhaW46CiAJCSAqIGxvY2sgdG8gaW5zcGVjdCB0aGUgcGFnZSBMU04sIHNvIHRoaXMgY2FuJ3Qg
YmUgZG9uZSBpbnNpZGUKIAkJICogU3RyYXRlZ3lHZXRCdWZmZXIuCiAJCSAqLwotCQlpZiAoc3Ry
YXRlZ3kgIT0gTlVMTCkKKwkJaWYgKHN0cmF0ZWd5ICYmIGZyb21fcmluZyAmJgorCQkJQnVmZmVy
TmVlZHNXQUxGbHVzaChidWZfaGRyLCBidWZfc3RhdGUsIGZhbHNlKSAmJgorCQkJU3RyYXRlZ3lS
ZWplY3RCdWZmZXIoc3RyYXRlZ3ksIGJ1Zl9oZHIsIGZyb21fcmluZykpCiAJCXsKLQkJCVhMb2dS
ZWNQdHIJbHNuOwotCi0JCQkvKiBSZWFkIHRoZSBMU04gd2hpbGUgaG9sZGluZyBidWZmZXIgaGVh
ZGVyIGxvY2sgKi8KLQkJCWJ1Zl9zdGF0ZSA9IExvY2tCdWZIZHIoYnVmX2hkcik7Ci0JCQlsc24g
PSBCdWZmZXJHZXRMU04oYnVmX2hkcik7Ci0JCQlVbmxvY2tCdWZIZHIoYnVmX2hkcik7Ci0KLQkJ
CWlmIChYTG9nTmVlZHNGbHVzaChsc24pCi0JCQkJJiYgU3RyYXRlZ3lSZWplY3RCdWZmZXIoc3Ry
YXRlZ3ksIGJ1Zl9oZHIsIGZyb21fcmluZykpCi0JCQl7Ci0JCQkJTG9ja0J1ZmZlcihidWYsIEJV
RkZFUl9MT0NLX1VOTE9DSyk7Ci0JCQkJVW5waW5CdWZmZXIoYnVmX2hkcik7Ci0JCQkJZ290byBh
Z2FpbjsKLQkJCX0KKwkJCUxvY2tCdWZmZXIoYnVmLCBCVUZGRVJfTE9DS19VTkxPQ0spOworCQkJ
VW5waW5CdWZmZXIoYnVmX2hkcik7CisJCQlnb3RvIGFnYWluOwogCQl9CiAKIAkJLyogT0ssIGRv
IHRoZSBJL08gKi8KQEAgLTM0MzcsNiArMzQzMCw1MSBAQCBUcmFja05ld0J1ZmZlclBpbihCdWZm
ZXIgYnVmKQogCQkJCQkJCSAgQkxDS1NaKTsKIH0KIAorLyoKKyAqIFJldHVybnMgdHJ1ZSBpZiB0
aGUgYnVmZmVyIG5lZWRzIFdBTCBmbHVzaGVkIGJlZm9yZSBpdCBjYW4gYmUgd3JpdHRlbiBvdXQu
CisgKgorICogVGhlIGNhbGxlciBtdXN0IHBhc3MgYSBidWZfc3RhdGUgdmFsdWUgcmVhZCBmcm9t
IGJ1ZmRlc2MtPnN0YXRlLgorICoKKyAqIElmIHRoZSByZXN1bHQgaXMgcmVxdWlyZWQgdG8gYmUg
Y29ycmVjdCwgdGhlIGNhbGxlciBtdXN0IGhvbGQgYSBidWZmZXIKKyAqIGNvbnRlbnQgbG9jay4g
SWYgdGhleSBvbmx5IGhvbGQgYSBzaGFyZWQgY29udGVudCBsb2NrLCB3ZSdsbCBuZWVkIHRvCisg
KiBhY3F1aXJlIHRoZSBidWZmZXIgaGVhZGVyIHNwaW5sb2NrLCBzbyB0aGV5IG11c3Qgbm90IGFs
cmVhZHkgaG9sZCBpdC4KKyAqLworc3RhdGljIGJvb2wKK0J1ZmZlck5lZWRzV0FMRmx1c2goQnVm
ZmVyRGVzYyAqYnVmZGVzYywgdWludDY0IGJ1Zl9zdGF0ZSwKKwkJCQkJYm9vbCBleGNsdXNpdmVf
bG9ja2VkKQoreworCUJ1ZmZlcgkJYnVmZmVyOworCWNoYXIJICAgKnBhZ2U7CisJWExvZ1JlY1B0
cglsc247CisKKwkvKgorCSAqIFVubG9nZ2VkIGJ1ZmZlcnMgY2FuJ3QgbmVlZCBXQUwgZmx1c2gu
IFNlZSBGbHVzaEJ1ZmZlcigpIGZvciBtb3JlCisJICogZGV0YWlscyBvbiB1bmxvZ2dlZCByZWxh
dGlvbnMgd2l0aCBMU05zLgorCSAqLworCWlmICghKGJ1Zl9zdGF0ZSAmIEJNX1BFUk1BTkVOVCkp
CisJCXJldHVybiBmYWxzZTsKKworCWJ1ZmZlciA9IEJ1ZmZlckRlc2NyaXB0b3JHZXRCdWZmZXIo
YnVmZGVzYyk7CisJQXNzZXJ0KCFCdWZmZXJJc0xvY2FsKGJ1ZmZlcikpOworCXBhZ2UgPSBCdWZm
ZXJHZXRQYWdlKGJ1ZmZlcik7CisKKwlpZiAoZXhjbHVzaXZlX2xvY2tlZCkKKwl7CisJCUFzc2Vy
dChCdWZmZXJJc0xvY2tlZEJ5TWVJbk1vZGUoYnVmZmVyLCBCVUZGRVJfTE9DS19FWENMVVNJVkUp
KTsKKwkJbHNuID0gUGFnZUdldExTTihwYWdlKTsKKwl9CisJZWxzZQorCXsKKwkJLyogQnVmZmVy
IGlzIGVpdGhlciBzaGFyZSBsb2NrZWQgb3Igbm90IGxvY2tlZC4gKi8KKwkJTG9ja0J1Zkhkcihi
dWZkZXNjKTsKKwkJbHNuID0gUGFnZUdldExTTihwYWdlKTsKKwkJVW5sb2NrQnVmSGRyKGJ1ZmRl
c2MpOworCX0KKworCXJldHVybiBYTG9nTmVlZHNGbHVzaChsc24pOworfQorCisKICNkZWZpbmUg
U1RfU09SVCBzb3J0X2NoZWNrcG9pbnRfYnVmZmVyaWRzCiAjZGVmaW5lIFNUX0VMRU1FTlRfVFlQ
RSBDa3B0U29ydEl0ZW0KICNkZWZpbmUgU1RfQ09NUEFSRShhLCBiKSBja3B0X2J1Zm9yZGVyX2Nv
bXBhcmF0b3IoYSwgYikKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3N0b3JhZ2UvYnVmZmVyL2Zy
ZWVsaXN0LmMgYi9zcmMvYmFja2VuZC9zdG9yYWdlL2J1ZmZlci9mcmVlbGlzdC5jCmluZGV4IGI3
Njg3ODM2MTg4Li42ZjMxY2QyMjI4OSAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvc3RvcmFnZS9i
dWZmZXIvZnJlZWxpc3QuYworKysgYi9zcmMvYmFja2VuZC9zdG9yYWdlL2J1ZmZlci9mcmVlbGlz
dC5jCkBAIC03ODYsNiArNzg2LDggQEAgSU9Db250ZXh0Rm9yU3RyYXRlZ3koQnVmZmVyQWNjZXNz
U3RyYXRlZ3kgc3RyYXRlZ3kpCiBib29sCiBTdHJhdGVneVJlamVjdEJ1ZmZlcihCdWZmZXJBY2Nl
c3NTdHJhdGVneSBzdHJhdGVneSwgQnVmZmVyRGVzYyAqYnVmLCBib29sIGZyb21fcmluZykKIHsK
KwlBc3NlcnQoc3RyYXRlZ3kpOworCiAJLyogV2Ugb25seSBkbyB0aGlzIGluIGJ1bGtyZWFkIG1v
ZGUgKi8KIAlpZiAoc3RyYXRlZ3ktPmJ0eXBlICE9IEJBU19CVUxLUkVBRCkKIAkJcmV0dXJuIGZh
bHNlOwpAQCAtNzk2LDcgKzc5OCw3IEBAIFN0cmF0ZWd5UmVqZWN0QnVmZmVyKEJ1ZmZlckFjY2Vz
c1N0cmF0ZWd5IHN0cmF0ZWd5LCBCdWZmZXJEZXNjICpidWYsIGJvb2wgZnJvbV9yCiAJCXJldHVy
biBmYWxzZTsKIAogCS8qCi0JICogUmVtb3ZlIHRoZSBkaXJ0eSBidWZmZXIgZnJvbSB0aGUgcmlu
ZzsgbmVjZXNzYXJ5IHRvIHByZXZlbnQgaW5maW5pdGUKKwkgKiBSZW1vdmUgdGhlIGRpcnR5IGJ1
ZmZlciBmcm9tIHRoZSByaW5nOyBuZWNlc3NhcnkgdG8gcHJldmVudCBhbiBpbmZpbml0ZQogCSAq
IGxvb3AgaWYgYWxsIHJpbmcgbWVtYmVycyBhcmUgZGlydHkuCiAJICovCiAJc3RyYXRlZ3ktPmJ1
ZmZlcnNbc3RyYXRlZ3ktPmN1cnJlbnRdID0gSW52YWxpZEJ1ZmZlcjsKLS0gCjIuNDMuMAoK
--000000000000a31c31064c72904d--