Restructured Shared Buffer Hash Table

On 7 Jul 2026, at 23:41, Dhruv Aron <dhruv.aron@gmail.com> wrote:

<restructured_shared_buffer_table.patch>

Hi,

Very interesting work!

I benchmarked the `restructured_shared_buffer_table.patch`. The machine
had 16 vCPUs and 32 GB of RAM; both builds used -O2 without assertions. The
numbers below are medians of five paired 30-second runs, shown as patched/base
TPS deltas. Runs above 64 clients used 64 pgbench jobs. No checkpoints ran
during the measured intervals.

For select-only workloads I got:

clients
scale / shared_buffers 1 16 64 128 256 512
100 / 4 GB (fits) +1.75% +0.76% +2.19% - - -
500 / 512 MB (larger) +1.03% +0.75% +1.11% - - -
5 / 256 MB (fits) - - - +1.10% +0.84% +1.84%
100 / 128 MB (larger) - - - +1.22% - -0.45%

I also tested fresh scale-100 clusters with 128 MB of shared buffers:

workload 1 16 64 256 clients
select-only +1.52% -0.08% +1.01% -2.56%
simple-update +0.34% -1.32% -3.59% -1.30%
TPC-B-like -0.29% +1.65% +2.31% +1.82%

The read-write results are noisy. The select-only tests show a small improvement
in most configurations. Perhaps, simple-update worth deeper investigation.

Thanks!

Best regards, Andrey Borodin

On Wed, Aug 5, 2026 at 9:02 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

Can you share the benchmark you used, please?

Please see the attached lock_hold_benchmark.diff file; the bench_collide.sh
and compare_collide.sh scripts are utilized to measure the average time
under the spinlock.

Is that a problem or not? Can you come up with an adversary test case where

those conflicts cause a lot of spinning?

I do not believe it is a problem. Since the change is in
InvalidateBuffer(), the extra spinning can only occur during DROP/TRUNCATE
operations, which should in turn execute while holding the appropriate
AccessExclusiveLock for the affected relation(s). As such, the worst case
is most likely just clock-sweep and bgwriter collisions during a large
DROP/TRUNCATE, which should not result in much contention and additional
spinning.

On Thu, Aug 6, 2026 at 2:07 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Perhaps, simple-update worth deeper investigation.

I attempted to replicate your setup (16 vCPU’s, scale-100 / 128 MB, six
paired 30-second runs) and run the same workloads, though I am unable to
reproduce those exact numbers:

*Clients* *Patched/Base TPS Delta (%)*
16 +1.60%
64 +1.01%
256 +1.13%

I assume that these numbers have some variance, so I ran the same setup
with a slightly higher sample size (thirty paired 20-second runs instead,
bracketed pairs indicate a 95% bootstrap confidence interval):

*Workload*                    *1*
*16*                                       *64*
             *256 Clients*
select-only                +0.38% [-0.12, +0.66]       +0.56% [+0.35,
+1.40]       +0.13% [-0.13, +0.28]          +0.70% [+0.12, +1.42]
simple-update           +0.30% [-0.68, +0.80]       +0.30% [-0.75, +1.25]
      -0.16% [-0.41, +0.23]          +0.19% [-1.19, +1.91]
TPC-B-like                +0.65% [+0.07, +1.45]      +0.46% [-3.44, +5.30]
      +0.29% [-0.28, +0.66]          +0.18% [-1.03, +1.47]

In all given cases, there appears to be no statistically significant TPS
difference between dynahash and the restructured table in the patch at
worst, with the restructured table looking to be slightly better in the
best scenario(s).

Best,
Dhruv Aron

+Andres Freund <andres@anarazel.de>, I quote you below

Hi Dhruv,

On 07/07/2026 21:41, Dhruv Aron wrote:

I would like to emphasize that I think the changes
here offer enough standalone benefits to merit their own patch.

Agreed, I am looking forward to the patch.

LOCKING

Heikki Linnakangas hlinnaka@iki.fi wrote at Jul 7, 2026, 8:14 PM

Hmm, we're now holding the buffer header lock much longer than before,
in InvalidateBuffer(). It's a spinlock, it really should not be held for
more than a few instructions.

Is that paraphrasing storage/buffer/README?

* Each buffer header contains a spinlock that must be taken when examining
or changing fields of that buffer header. This allows operations such as
ReleaseBuffer to make local state changes without taking any system-wide
lock. We use a spinlock, not an LWLock, since there are no cases where
the lock needs to be held for more than a few instructions.

The problem with long locks is that it will keep other processes busy during
contention, but since this is locking a single buffer, contention should be
unlikely, and thus the effect should be small.

DELETION OPTIMISATION OPPORTUNITY

On 07/07/2026 21:41, Dhruv Aron wrote:

it enforces the invariant that *entries[x]* describes the page in buffer

*x*.

so this leads to two potential optimisations

The first being using the buffer id to delete.
 void
- BufTableDelete(BufferTag *tagPtr, uint32 hashcode)
+ BufferTableDeleteId(BufferTa , uint32 hashcode)
   while(id != BUF_TABLE_CHAIN_END)
-    if (BufferTagsEqual(&entries[id].tag, tagPtr))
+   if (id == del_id))

The other would be to use a doubly linked list, increasing the entry
memory footprint from 24 to 28 bytes, the deletion would require
no iteration.

+ if(entries[id].prev != BUF_TABLE_CHAIN_END)
+ entries[entries[id].prev].next = entries[id].next
+ else buckets[bucket].head = entries[id].next

MEMORY USAGE ANALYSIS

On Jul 7, 2026 at 10:12 PM Heikki Linnakangas wrote

unpatched master, with shared_buffers='128 MB':
name | off | size | allocated_size
Shared Buffer Lookup Table | 141607040 | 926000 | 926108
With this patch:
Shared Buffer Lookup Buckets | 141607040 | 65536 | 65644
Shared Buffer Lookup Entries | 141672576 | 393216 | 393216

So the new hash table takes much less memory. That's nice because you
can then fit more in CPU caches.

with 128MB the master uses 0x8000 buckets, patched
uses 0x4000. Because 128MB = 0x4000 * 8192 master
passes .nelems = 0x4000 + NUM_BUFFER_PARTITIONS
increasing to the next power of two 0x8000.

+static inline int
+BufTableNumBuckets(void)
+{
+    return Max(NUM_BUFFER_PARTITIONS, pg_nextpower2_32(NBuffers));
+}
+ num_buckets = BufTableNumBuckets();
-    size = NBuffers + NUM_BUFFER_PARTITIONS;
-
-    ShmemRequestHash(.name = "Shared Buffer Lookup Table",
-                     .nelems = size,
-                     .ptr = &SharedBufHash,
-                     .hash_info.keysize = sizeof(BufferTag),
-                     .hash_info.entrysize = sizeof(BufferLookupEnt),
-                     .hash_info.num_partitions = NUM_BUFFER_PARTITIONS,
-                     .hash_flags = HASH_ELEM | HASH_BLOBS | HASH_PARTITION
| HASH_FIXED_SIZE,
+    ShmemRequestStruct(.name = "Shared Buffer Lookup Buckets",
+                       .size = (Size) num_buckets *
sizeof(BufferLookupBucket),
+                       .ptr = (void **) &buckets,
+        );
+
+    ShmemRequestStruct(.name = "Shared Buffer Lookup Entries",
+                       .size = (Size) NBuffers * sizeof(BufferLookupEnt),
+                       .ptr = (void **) &entries,Element size on 64-bit
machines

This slight misalignment might have played against the patched
speed. Because for the master a power of two is at ~0.5 entry/bucket
rate, and the patched version is at ~1 entry/bucket.

BENCHMARK

+ int64 j = ord[i];
+ BufTableInsert(&ptag[j], phash[j], bufids[j]);
...
+ sink += BufTableLookup(&ptag[j], phash[j]);
...
+ sink += BufTableLookup(&atag[j], ahash[j]);
...
+ BufTableDelete(&ptag[j], phash[j]);

The benchmark covers a very particular case. The indices
are shuffled but insert, lookup and delete, all use the same permutation.

As a consequence of this. The lookup_hit test is always going to find
each entry before its next entry in the bucket. The delete will always
find the entry being deleted at bucket head. For lookup_miss it doesn't
matter. It might seem hypocritical of me as I did exactly that when I
worked on buffer pinning [1]/messages/by-id/rfjyce5hmfkp2pbgjaxvmc76zy33kpokigbkwnounxfmz6uyd5@vt7yxibmfy6n. But in that case I was simulating
prefetching, where pins follow roughly a fifo (or fpfu) order.

MEMORY USAGE ANALYSIS

+ elementSize = MAXALIGN(sizeof(HASHELEMENT)) + MAXALIGN(hctl->entrysize);
With .hash_info.keysize = sizeof(BufferTag) = 20
And MAXALIGN defined as the minimum multiple of 8 not smaller than the
input.
it was MAXALIGN(12) + MAXALIGN(20) = 16 + 24 = 40
assuming buckets = entries we would have another 8 bytes per bucket head
and 8 bytes per segment, assuming buckets = 2 * entries we would have
something like
56 + 2*8/256, not too far from allocated size.

128MB = 16384 entries, 926108 / 16384 = 56.52

Entries in the dynhash account for up to
dir 0x8000 / 0x100 pointers = 1024
bucket heads 0x8000 pointers = 262144
and for the entries 0x4000 * 40 = 655360
918528 bytes close enough from the number quoted at the top

In the patched version
0x4000 * 4 for indices = 65536
0x4000 * 24 for entries = 39216

THE REPLACED CODE

Here I try to summarise what is being removed, it makes
a lot of sense that it is faster.

1. external call hash_search_with_hash_value
2. freelist_idx: always computed, not used for lookup,
pointer dereference+branch
(((hctl)->num_partitions != 0) ? (hashvalue) % 32 : 0)
3. if (action ~ HASH_ENTER_*)
branch and unlikely split
4. hash_initial_lookup:
4.1 calc_bucket: a branch and one or two pointer dereferences.
4.2 hashp->dir[bucket >> 8].?[bucket & 255]: two pointer dereferences
and a branch (null check)
5. match function pointer, and keysize pointer references.
6 loop
5.1 currBucket->hashvalue == hashvalue possibly skipping 5.2
5.2 call match function by pointer (new version uses inline
BufferTagsEqual).
6. update *foundPtr
7. switch (multiple branches?)
8. for insert/delete again pointer dereference + branch twice
around SpinLock(Acquire/Release)

The only bit of performance that was dropped is checking the hash code
before the tag comparison. That could be a win if we think of comparing
32-bit then branching, successively.
But if it is implemented with vector optimisation, the BufferTagEqual
alone will probably be faster than trying to branch over it, not to mention
the need to store the hash.

[1]: /messages/by-id/rfjyce5hmfkp2pbgjaxvmc76zy33kpokigbkwnounxfmz6uyd5@vt7yxibmfy6n
/messages/by-id/rfjyce5hmfkp2pbgjaxvmc76zy33kpokigbkwnounxfmz6uyd5@vt7yxibmfy6n

Hi,

On 2026-07-07 11:41:47 -0700, Dhruv Aron wrote:

At Databricks, we’ve found that the existing dynahash table structure is
leaving performance gains on the table when it comes to shared buffer
lookups: the multi-level structure (directory, segment, bucket chain,
freelist) appears excessive for the shared buffers and could be simplified
to boost performance and lower memory overhead. As such, we are proposing a
specialized hash table just for this purpose and would appreciate feedback
on this approach.

Agreed, it's quite terrible.

To give a brief overview, our new table operates primarily on two arrays,
one for the entries and one for the bucket heads, and it enforces the
invariant that *entries[x]* describes the page in buffer *x*. Each entry
stores only a BufferTag and a ‘next’ index (representing the next entry in
the same bucket chain), with each bucket head only storing a ‘head’ index
(representing the first entry in the bucket chain). At a high-level, the
table essentially creates a logical linked list for each bucket on top of
the flat physical arrays.

Why did you choose a design that is effectively pointered? Pointered conflict
handling tends to be a good bit slower due to the typically unprefetchable
accesses. That's also - I suspect at least - part of what's pushing you
towards having both buckets[] and entries[], which seems like an unnecessary
indirection to me. Although admittedly the partition locking handling would
be more complicated without the separate array.

I also suspect that it'd be better to store a hash value in the buckets,
BufferTagsEqual() is decidedly not cheap, and you'll obviously get a lot of
"false" matches from hashcode % num_buckets that would be much cheaper to
detect with a stored hash value.

My higher level problems with the current architecture of the buffer mapping
infrastructure are the following:

1) We need a way to iterate over all buffers for a relation in an efficient
way

The fact that today stuff like dropping storage requires scanning all of
the buffer pool is probably the most problematic for using decently sized
buffer pools. We've made it a bit less bad by combining multiple such
scans into one, but fundamentally it's still O(NBuffers).

2) I think any buffer mapping lookup datastructure with 20 byte keys is going
to considerably not great for performance.

3) We should have efficient ordered lookup, to make things like "are there any
not-present blocks in the next N blocks" cheap. Today we need to do full
buffer lookups for readahead, which requires us to be very minimal about
lookahead when having a high cache hit ratio, to avoid performance
regressions - but that also prevents us from avoiding synchronous misses in
such cases.

4) Acquiring a lock for every lookup scales badly on larger machines, even if
the lock itself is not contended, due to the cacheline contention it
creates

5) The datastructure should benefit from spatial locality

It's much more common for subsequent buffer mapping lookups to look up
nearby blocks than blocks very far away. But with hash tables, the
likelihood of finding blocks N and N+1 in the CPU cache is no better than
looking up two entirely independent blocks.

For 1-3), I think we should move towards a two-layer datastructure:

a) a mapping from relation+fork to a per-"logical file" datastructure

Keyed by database, tablespace, relfilenode, fork (although the fork
could be handled differently).

This lookup would be cached somewhere below Relation, so we only would need
to occasionally do it, so the size of the key would not matter for
performance.

This addresses 1+2.

(there's plenty complexity here, don't get me wrong)

b) A block-number keyed lookup datastructure returning buffer IDs

If this datastructure is ordered, it addresses 3).

Due to the small key, something like a radix tree is viable (not a plain
one, but something like what we have in radixtree.h, with the missing
optimization from the referenced paper added).

A radix tree would also address 5).

To address 4) I think we should eventually allow to make lookups in b)
lock-free, using something like RCU or EBR (arguably a form of RCU). I would
definitely not tackle that at the same time, but I think it's worth keeping in
mind.

I'm of a somewhat split mind about improving the efficiency of the current
design without addressing any of the architectural problems. It's of course
nice to make it faster, but it also takes bandwidth that can't be spent on
the architectural problems...

The attached patch implements this functionality and passes the existing
regression tests; the buf_table.c functions were modified directly, with
bufmgr.c also changed slightly to prevent a race condition.

As mentioned elsewhere, it's not OK to keep spinlocks held across nontrivial
operations, which both lwlocks and hash table lookup certainly are.

Sspinlocks (neither plain ones nor the buffer header lock variant) have no
error recovery whatsoever, so any error that is thrown will make the system
unusable. It's hard to guarantee that nothing can throw an error unless you
keep the covered code very small. Adding error recovery (like lwlocks have
via LWLockReleaseAll()), would make spinlocks slower.

In this case I'm pretty sure this is also a undetected deadlock, as other
places acquire the buffer header spinlock while holding the buffer partition
lock. You can't just change the nesting in one place, you'd have to change it
everywhere (but don't, I'm quite certain that we're never going to allow
holding spinlocks that long).

I don't really understand the race condition this is trying to address:

+	/* Unlock buffer header after the entry is deleted to avoid a race condition:
+	 * If unlocked prior, a concurrent GetVictimBuffer() could insert a new entry
+	 * for the same buffer and overwrite the entry slot. Then, the BufTableDelete()
+	 * would be unable to find the entry and would corrupt the hashtable. */
+	UnlockBufHdrExt(buf, buf_state,
+		0,
+		BUF_FLAG_MASK | BUF_USAGECOUNT_MASK,
+		0);

How could there be a concurrent insertion while the buffer partition lock is
held?

Also, GetVictimBuffer() doesn't insert anything into the buffer mapping table,
it just calls InvalidateVictimBuffer(), which deletes from the buffer mapping
table?

Is the concern that two GetVictimBuffer() calls landing on the same buffer
would be a problem? If so, I don't see the problem, at least one of the
GetVictimBuffer()s would fail due to the refcount in InvalidateVictimBuffer()
being seen as != 1.

My testing (helper script also attached) indicates that all three standard
hash table operations (insert, lookup, and delete) generally execute
significantly faster than the existing PG18 dynahash counterparts:

FWIW, on machines with the necessary hardware support, postgres' instr_time.h
should now be quite fast, it's using rdtsc[p] if available.

Greetings,

Andres Freund

On Tue, Aug 11, 2026 at 9:50 PM Andres Freund <andres@anarazel.de> wrote:

Sspinlocks (neither plain ones nor the buffer header lock variant) have no
error recovery whatsoever, so any error that is thrown will make the

system

unusable.

This is a much better reason for not doing complex stuf under the spin lock,
not the number of instruction, but guarantees of safety.

I don't really understand the race condition this is trying to address:

+ /* Unlock buffer header after the entry is deleted to avoid a

race condition:

My guess:
Assume we have one bucket a: [a1, a2, a3], and bucket b: [b1]
- invalidate a1
- unlock a1
- reuse a1 as b2
now bucket a will see [a1, b2]
deletion will fail to find a2
Now buckets a and b are linked (in an invalid way).

You can't just change the nesting in one place, you'd have to change it
everywhere

Dhruv's patch doesn't change the nesting order. But that is an important
point.

Regards,
Alexandre

On Tue, Aug 11, 2026 at 10:50 PM Andres Freund <andres@anarazel.de> wrote:

...

My higher level problems with the current architecture of the buffer mapping
infrastructure are the following:

...
2) I think any buffer mapping lookup datastructure with 20 byte keys is going
to considerably not great for performance.

...
...

For 1-3), I think we should move towards a two-layer datastructure:

a) a mapping from relation+fork to a per-"logical file" datastructure

Keyed by database, tablespace, relfilenode, fork (although the fork
could be handled differently).

This lookup would be cached somewhere below Relation, so we only would need
to occasionally do it, so the size of the key would not matter for
performance.

This addresses 1+2.

(there's plenty complexity here, don't get me wrong)

Maybe a higher-level solution could be in order here, like allocating
a cluster-wide (i.e postmaster-wide) global oid at object creation
(and assign new ones after create database via copy) and just use that
?

As you said, the fork may be keyed differently, but if we accept the
global object count rduction from 4 billion to 256 million we could
use 4 bits of the global oid to encode the fork nr.

This would move all the complexity of generating the (database,
tablespace, relfilenode, fork) -> id mapping to table ceation time and
later it would just sit there and be always available

Hi Dhruv,

I am formatting your patch, and doing a few minor adjustments

It addresses Andres Freund's feedback

FWIW, on machines with the necessary hardware support, postgres'

instr_time.h

should now be quite fast, it's using rdtsc[p] if available.

This version uses instr_time and a a little bit of pre-processing to write
the
benchmark as

+ BEGIN_TIMING("insert", n)
+     int32 j = w_ord[i];
+     BufTableInsert(&ptag[j], phash[j], bufids[j]);
+ END_TIMING
+
+ BEGIN_TIMING("hit", n)
+     int32 j = r_ord[i];
+     sink += BufTableLookup(&ptag[j], phash[j]);
+ END_TIMING

I also increased the number of buckets to make 1/3 < occupation < 2/3
instead of 1/2 < occupation < 1. 100% occupation could produce longer
chains (not critical though, as it is not using open addressing).

I don't really understand the race condition this is trying to address:

+ /* Unlock buffer header after the entry is deleted to avoid a race

condition:

+ * If unlocked prior, a concurrent GetVictimBuffer() could insert

a new entry

+ * for the same buffer and overwrite the entry slot. Then, the

BufTableDelete()

+ * would be unable to find the entry and would corrupt the

hashtable. */

+     UnlockBufHdrExt(buf, buf_state,
+             0,
+             BUF_FLAG_MASK | BUF_USAGECOUNT_MASK,
+             0);

How could there be a concurrent insertion while the buffer partition lock

is

held?

I am removing that part as there is a consensus among the big guys that
we shouldn't hold a spin-lock while doing the BufferTableDelete.

I think I could explain why this is necessary, but I want to see your
argument.
Was it safe before? if so, what is the property of dynhash that we lost on
this
patch.

I could argue that BufferTableDelete is safe if we check for the bounds of
.next and make sure there are no cycles in the chain. But again I will let
you elaborate that argument.

Regards,
Alexandre

On Thu, Aug 13, 2026 at 12:52 PM Alexandre Felipe <
o.alexandre.felipe@gmail.com> wrote:

I don't really understand the race condition this is trying to address:

+ /* Unlock buffer header after the entry is deleted to avoid a

race condition:

+ * If unlocked prior, a concurrent GetVictimBuffer() could insert

a new entry

+ * for the same buffer and overwrite the entry slot. Then, the

BufTableDelete()

+ * would be unable to find the entry and would corrupt the

hashtable. */

+     UnlockBufHdrExt(buf, buf_state,
+             0,
+             BUF_FLAG_MASK | BUF_USAGECOUNT_MASK,
+             0);

How could there be a concurrent insertion while the buffer partition lock

is

held?

I am removing that part as there is a consensus among the big guys that
we shouldn't hold a spin-lock while doing the BufferTableDelete.

I think I could explain why this is necessary, but I want to see your
argument.
Was it safe before? if so, what is the property of dynhash that we lost on
this
patch.

I could argue that BufferTableDelete is safe if we check for the bounds of
.next and make sure there are no cycles in the chain. But again I will let
you elaborate that argument.

Regards,
Alexandre

Apologies for the inadequate race condition description in the comment.
Without the spinlock change, a concurrent backend can overwrite the entry
slot that is supposed to be deleted imminently after the header lock is
released, corrupting the bucket chains.
Consider two concurrent backends b1 and b2 in the following scenario:

1. b1 executes within InvalidateVictimBuffer(), clearing the header for
buffer *x* and unlocking the header lock.
2. b2 calls GetVictimBuffer() which returns *x*, and b2 proceeds to
insert a new entry into entry slot *x* via BufTableInsert(). This
overwrites the original entry in that slot and breaks the bucket chain the
original entry was part of.
3. b1 continues with BufTableDelete() to delete the entry that is no
longer in slot *x* (since b2 overwrote it) and is unable to find it.

I do not believe simply following the .next indices and checking for cycles
is sufficient for two main reasons:

1. The entry *p* that used to chain to entry *x* through its .next still
also points to the new entry *x*. If the new entry *x* hashes to a
different bucket than entry *p *and the original entry *x*, following
entry *p* to the new entry *x* would jump buckets (which is incorrect)
and potentially bypass the partition locking paradigm, causing further
concurrency issues.
2. The entries that were once reached by following the .next of the
original entry *x* are now unreachable, causing false lookup misses.

This is not an issue with dynahash because it allocates new entries via the
freelist, which safely allows inserting a new entry while the stale one
waits to be deleted.
Since the patched table instead maintains that entry *x* describes buffer
*x*, we cannot have both the new entry and the stale one present
simultaneously; the old one must be fully deleted (with the bucket chains
adjusted appropriately) before the new one is inserted.
As such, the patched code is unsafe without the lock change, though I admit
that holding the lock while calling BufTableDelete is not ideal and other
solutions should be explored.

Best,
Dhruv Aron

Thank you Dhruv,

Let me distil your answer

On Thu, Aug 13, 2026 at 11:49 PM Dhruv Aron <dhruv.aron@gmail.com> wrote:

Without the spinlock change, a concurrent backend can overwrite the entry
slot that is supposed to be deleted imminently after the header lock is
released, corrupting the bucket chains.

And

This is not an issue with dynahash because it allocates new entries via
the freelist, which safely allows inserting a new entry while the stale one
waits to be deleted.

------

Simple idea

An alternative would be to pin the buffers instead.

* StrategyGetBuffer
*
* Called by the bufmgr to get the next candidate buffer to use in
* GetVictimBuffer(). The only hard requirement GetVictimBuffer() has is
that
* the selected buffer must not currently be pinned by anyone.

InvalidateVictimBuffer is safe, because the buffer is pinned. But
InvalidateBuffer
is not, as it deletes while refcount==0, when a concurrent GetVictimBuffer
could
take it.

Ambitious idea (food for thought):

What if we don't remove it from the BufTable, WaitIO if needed,
Reset BUF_USAGECOUNT_MASK and return?
No LWLock required, the buffer would be removed from BufTable later
by InvalidateVictimBuffer.

Maybe we could get rid of the buffer scan too?
Lock the relation so that no one can start any IO on it.
Wait for every I/O in progress.
Ready

Just let them be reused naturally on on the next clock sweeps

Andres, do you want to educate us as to why the above ideas could be
terrible?

Regards,
Alexandre

On Tue, Jul 7, 2026 at 8:14 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

bufmgr.c also changed slightly to prevent a race condition.

Hmm, we're now holding the buffer header lock much longer than before,
in InvalidateBuffer(). It's a spinlock, it really should not be held for
more than a few instructions. BufTableDelete() is very fast in the new

implementation, but still.

As Andres later clarified the problem is not the duration of the lock, is
the complexity of the safety risk of what we do under it.

Could we perhaps do some of

BufTableDelete()'s work ahead of time, before we acquire the buffer
header lock?

This is a better approach in my opinion, we can do the buffer search before
acquiring the spin-lock and just break the chain link under the spin-lock.
It is easier to prove the safety of the later operation, and hopefully get
an
agreement on the suitability of the change.

I am adding a patch on top of Dhruv's work with that change.

I also implemented the deletion by Buffer (del-buf), about 30% faster than
original deletion by BufferTag (del-tag).

I added the buffer header lock (spin-lock), partition lock (LWLock),
and BufferHashCode to the benchmark. To see when we are entering
the diminishing returns zone.

0003 - 2-step deletion (indistinguishable from 0002), del-buf faster than
del-tag.

op | avg | min | [q1 | median | q3] | p99 | std
-----------+------+------+------+--------+------+------+------
insert | 4.45 | 4.30 | 4.36 | 4.37 | 4.41 | 5.56 | 0.28
hit | 5.96 | 5.81 | 5.86 | 5.88 | 5.93 | 6.82 | 0.27
del-buf | 4.95 | 4.86 | 4.88 | 4.90 | 4.94 | 5.55 | 0.17
insert | 4.46 | 4.39 | 4.41 | 4.42 | 4.45 | 4.86 | 0.10
miss | 5.96 | 5.83 | 5.88 | 5.90 | 5.96 | 6.52 | 0.15
del-tag | 7.32 | 7.14 | 7.18 | 7.20 | 7.28 | 8.49 | 0.61
LWLock-ex | 7.88 | 7.78 | 7.78 | 7.79 | 7.88 | 8.70 | 0.23
LWLock | 7.88 | 7.77 | 7.78 | 7.79 | 7.87 | 8.58 | 0.20
HdrLock | 4.99 | 4.74 | 4.80 | 5.03 | 5.08 | 5.68 | 0.19
hash | 4.44 | 4.37 | 4.38 | 4.39 | 4.44 | 4.97 | 0.16
compare | 1.37 | 1.31 | 1.34 | 1.35 | 1.35 | 1.52 | 0.34
nop | 0.91 | 0.88 | 0.90 | 0.90 | 0.91 | 1.03 | 0.04

0002 - Inline hash (Dhruv's original patch)
op | avg | min | [q1 | median | q3] | p99 | std
-----------+------+------+------+--------+------+------+------
insert | 5.32 | 4.87 | 5.13 | 5.33 | 5.45 | 6.00 | 0.30
hit | 7.15 | 6.53 | 6.92 | 7.16 | 7.32 | 7.77 | 0.30
miss | 7.59 | 6.96 | 7.32 | 7.61 | 7.76 | 8.19 | 0.52
del-tag | 8.59 | 7.85 | 8.30 | 8.60 | 8.78 | 9.32 | 0.50
LWLock-ex | 8.43 | 7.78 | 8.15 | 8.46 | 8.63 | 9.17 | 0.31
LWLock | 8.44 | 7.78 | 8.15 | 8.47 | 8.64 | 9.12 | 0.32
HdrLock | 5.34 | 4.78 | 5.19 | 5.32 | 5.51 | 5.82 | 0.25
hash | 4.74 | 4.38 | 4.59 | 4.75 | 4.85 | 5.17 | 0.20
compare | 1.48 | 1.37 | 1.42 | 1.47 | 1.53 | 1.67 | 0.08
nop | 0.98 | 0.90 | 0.94 | 0.97 | 1.02 | 1.11 | 0.05

0001 - Benchmark (master)
op | avg | min | [q1 | median | q3] | p99 | std
-----------+-------+-------+-------+--------+-------+-------+------
insert | 16.28 | 15.48 | 15.65 | 15.75 | 16.18 | 23.91 | 2.61
hit | 13.31 | 12.51 | 12.72 | 12.76 | 12.99 | 17.45 | 7.70
miss | 13.29 | 12.65 | 12.94 | 12.98 | 13.20 | 16.34 | 2.21
del-tag | 10.31 | 9.66 | 9.96 | 9.99 | 10.15 | 15.33 | 1.72
LWLock-ex | 7.94 | 7.59 | 7.79 | 7.80 | 7.93 | 9.17 | 0.63
LWLock | 7.92 | 7.65 | 7.78 | 7.79 | 7.91 | 10.00 | 0.46
HdrLock | 5.01 | 4.65 | 4.81 | 5.03 | 5.10 | 5.70 | 0.32
hash | 4.56 | 4.39 | 4.47 | 4.51 | 4.57 | 5.18 | 0.20
compare | 1.40 | 1.34 | 1.37 | 1.37 | 1.39 | 1.68 | 0.09
nop | 0.92 | 0.88 | 0.90 | 0.90 | 0.92 | 1.10 | 0.05

Regards,
Alexandre