Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Do not lock in BufferGetLSNAtomic() on archs with 8 byte atomic reads

  2. Reduce pinning and buffer content locking for btree scans.

  1. Remove header lock BufferGetLSNAtomic() on architectures with 64 bit atomic operations

    Andreas Karlsson <andreas@proxel.se> — 2025-11-23T23:10:03Z

    Hi,

Andres pointed out this possible optimization on Discord so I hacked up 
a quick patch which avoids taking a lock when reading the LSN from a 
page on architectures where we can be sure to not get a torn value. It 
is always nice to remove a lock from a reasonably hot code path.

I thought about using our functions for atomics but did not do so since 
I did not want to introduce any extra overhead on platforms which do not 
support 64-bit atomic operations.

I decided to just remove the struct to make the code simpler and more 
consistent but I can also see an argument for keeping it to get some 
degree of type safety.

I have not properly benchmarked it yet but plan to do so when I am back 
from my vacation.

I have also included a cleanup patch where I change a macro into an 
inline function which I think improves code readability. Feel free to 
ignore that one if you want.

--
Andreas
Percona

  2. Re: Remove header lock BufferGetLSNAtomic() on architectures with 64 bit atomic operations

    Peter Geoghegan <pg@bowt.ie> — 2025-12-14T00:28:36Z

    On Sun, Nov 23, 2025 at 6:10 PM Andreas Karlsson <andreas@proxel.se> wrote:
> Andres pointed out this possible optimization on Discord so I hacked up
> a quick patch which avoids taking a lock when reading the LSN from a
> page on architectures where we can be sure to not get a torn value. It
> is always nice to remove a lock from a reasonably hot code path.

This seems very useful.

One reason why I'm interested in this work is that it will facilitate
other work (from Tomas Vondra and myself) on the proposed new
amgetbatch interface, which enables optimizations such as index
prefetching. That work will replace the use of the amgettuple
interface with a index page/batch oriented amgetbatch interface. It
generalizes nbtree's dropPin optimization (originally added under a
different name by commit 2ed5b87f) to work with any index AM that uses
the new amgetbatch interface -- which will include both nbtree and
hash in the initial version (and possibly other index AMs in the
future).

This means that there'll be quite a few new BufferGetLSNAtomic calls
during scans of hash indexes, where before there were none -- we need
to stash an LSN so that we have an alternative way of detecting unsafe
concurrent TID recycling when LP_DEAD-marking index tuples as a batch
is released (since there'll have been no buffer pin held on the index
leaf page while we accessed the heap when the dropPin optimization is
applied).

With page-level checksums enabled, on the recently posted v4 of our
patch set, I find that there's a regression of about 5% of transaction
throughput with a variant of pgbench SELECT that uses hash indexes.
But with your patch applied on top of our own, that regression
completely goes away.

FWIW the improvement I see with regular nbtree index scans is still
visible, but not quite as good as with hash indexes + the amgetbatch
patch set. It should be easy enough to see this for yourself. Standard
pgbench SELECT should work well enough -- just make sure that you test
with page-level checksums enabled.

-- 
Peter Geoghegan

  3. Re: Remove header lock BufferGetLSNAtomic() on architectures with 64 bit atomic operations

    Andreas Karlsson <andreas@proxel.se> — 2025-12-31T02:17:11Z

    On 11/24/25 12:10 AM, Andreas Karlsson wrote:
> Andres pointed out this possible optimization on Discord so I hacked up 
> a quick patch which avoids taking a lock when reading the LSN from a 
> page on architectures where we can be sure to not get a torn value. It 
> is always nice to remove a lock from a reasonably hot code path.
> 
> I thought about using our functions for atomics but did not do so since 
> I did not want to introduce any extra overhead on platforms which do not 
> support 64-bit atomic operations.
> 
> I decided to just remove the struct to make the code simpler and more 
> consistent but I can also see an argument for keeping it to get some 
> degree of type safety.

Noticed that the pageinspect tests in the CI did not like that I 
increased the alignment requirement of PageHeaderData from 4 to 8 bytes 
presumably due to a bytea's data being 4 byte aligned. I will need to 
think how to best fix this. Maybe copying the struct is fine in the the 
pageinspect code because being able to check for this alignment seems 
like a nice property for code which is not in pageinspect.

It gives us the following backtrace:

../src/include/storage/bufpage.h:397:16: runtime error: member access 
within misaligned address 0x55d6721e69bc for type 'const struct 
PageHeaderData', which requires 8 byte alignment
0x55d6721e69bc: note: pointer points here
   10 80 00 00 00 00 00 00  00 00 00 00 00 00 04 00  1c 00 e0 1f 00 20 
04 20  00 00 00 00 e0 9f 40 00
               ^
     #0 0x7fb39deb9b07 in PageGetMaxOffsetNumber 
../src/include/storage/bufpage.h:397
     #1 0x7fb39debac3e in heap_page_items 
../contrib/pageinspect/heapfuncs.c:168
     #2 0x55d63ff127c7 in ExecMakeTableFunctionResult 
../src/backend/executor/execSRF.c:234
     #3 0x55d63ff42aa7 in FunctionNext 
../src/backend/executor/nodeFunctionscan.c:94
     #4 0x55d63ff14157 in ExecScanFetch 
../src/include/executor/execScan.h:134
     #5 0x55d63ff14157 in ExecScanExtended 
../src/include/executor/execScan.h:195
     #6 0x55d63ff14157 in ExecScan ../src/backend/executor/execScan.c:59
     #7 0x55d63ff427b6 in ExecFunctionScan 
../src/backend/executor/nodeFunctionscan.c:269
     #8 0x55d63ff0c38c in ExecProcNodeFirst 
../src/backend/executor/execProcnode.c:469
     #9 0x55d63fef5cee in ExecProcNode 
../src/include/executor/executor.h:319
     #10 0x55d63fef6465 in ExecutePlan 
../src/backend/executor/execMain.c:1707
     #11 0x55d63fef7c00 in standard_ExecutorRun 
../src/backend/executor/execMain.c:366
     #12 0x55d63fef7c60 in ExecutorRun 
../src/backend/executor/execMain.c:303
     #13 0x55d640322295 in PortalRunSelect ../src/backend/tcop/pquery.c:916
     #14 0x55d64032534e in PortalRun ../src/backend/tcop/pquery.c:760
     #15 0x55d64031e34a in exec_simple_query 
../src/backend/tcop/postgres.c:1279
     #16 0x55d6403215ce in PostgresMain ../src/backend/tcop/postgres.c:4775
     #17 0x55d640316c6c in BackendMain 
../src/backend/tcop/backend_startup.c:124
     #18 0x55d6401b2711 in postmaster_child_launch 
../src/backend/postmaster/launch_backend.c:268
     #19 0x55d6401b7b19 in BackendStartup 
../src/backend/postmaster/postmaster.c:3598
     #20 0x55d6401ba061 in ServerLoop 
../src/backend/postmaster/postmaster.c:1713
     #21 0x55d6401bb972 in PostmasterMain 
../src/backend/postmaster/postmaster.c:1403
     #22 0x55d63ffde07b in main ../src/backend/main/main.c:231
     #23 0x7fb39e033ca7  (/lib/x86_64-linux-gnu/libc.so.6+0x29ca7) 
(BuildId: def5460e3cee00bfee25b429c97bcc4853e5b3a8)
     #24 0x7fb39e033d64 in __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x29d64) (BuildId: 
def5460e3cee00bfee25b429c97bcc4853e5b3a8)
     #25 0x55d63fb70260 in _start 
(/tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres+0x8b0260) 
(BuildId: d2e585ffc320b6de4bfce11bb801bb923e7d3484)

Links

- https://cirrus-ci.com/task/5353983260229632
- 
https://api.cirrus-ci.com/v1/artifact/task/5353983260229632/testrun/build/testrun/pageinspect/regress/log/postmaster.log

Andreas