Horton

Thread

Commits

  1. Make _bt_killitems drop pins it acquired itself.

  2. Avoid BufferGetLSNAtomic() calls during nbtree scans.

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

  1. strange perf regression with data checksums

    Tomas Vondra <tomas@vondra.me> — 2025-05-09T12:04:49Z

    Hi,

While running some benchmarks comparing 17 and 18, I ran into a simple
workload where 18 throughput drops by ~80%. After pulling my hair for a
couple hours I realized the change that triggered this is 04bec894a04c,
which set checksums on by default. Which is very bizarre, because the
workload is read-only and fits into shared buffers.

I've only observed this on large machines with 96+ cores, on azure, both
with Intel (Xeon 8370C) and AMD (EPYC 9V33X). I've not been successful
in reproducing it on the puny machines I have at home.

Let me demonstrate the issue.

1) Create a cluster, with an increased connection limit:

  pg_ctl -D data init
  echo 'max_connections = 100' >> data/postgresql.conf
  pg_ctl -D data -l pg.log start

Now the benchmark itself - it's fairly trivial, regular pgbench on scale
1, with an extra index on "pgbench_accounts.bid" column:

  createdb test
  pgbench -i -s 1 test
  psql test -c "create index on pgbench_accounts (bid)"

and a script with a simple query using the index

  select count(*) from pgbench_accounts where bid = 0

Cool, now let's get some numbers for 32-160 clients:

  for c in 32 64 96 128 160; do

    pgbench -n -f select.sql -T 10 -M prepared -c $c -j $c test | grep
'tps';

  done;


Which produces this:

  tps = 752464.727387 (without initial connection time)

  tps = 1062150.015297 (without initial connection time)

  tps = 572205.386159 (without initial connection time)

  tps = 568579.663980 (without initial connection time)

  tps = 561360.493639 (without initial connection time)

Clearly, at 96 clients the throughput just tanks. Now let's disable
checksums on the cluster:

  pg_ctl -D data -l pg.log stop

  pg_checksums --disable data

  pg_ctl -D data -l pg.log start


and run the script again

  tps = 753484.468817 (without initial connection time)

  tps = 1083752.631856 (without initial connection time)

  tps = 1862008.466802 (without initial connection time)

  tps = 1826484.489433 (without initial connection time)

  tps = 1818400.279069 (without initial connection time)


Clearly, the throughput does not drop, and it's ~3.5x higher. This is
from the Xeon machine, but I've seen the same thing on the EPYC boxes.
The impact varies, but in general it's 70-80%.

I'm not suggesting this is caused by 04bec894a04c, or even specific to
PG 18. I see the same issue on 17, except that 17 does not enable
checksums by default. For example on EPYC 9V74 the 17 does this:

   32   762187.724859
   64  1284731.279766
   96  2978828.264373
  128  2991561.835178
  160  2971689.161136

and with checksums

   32   874266.549870
   64  1286382.426281
   96   569647.384735
  128   562128.010301
  160   561826.908181

So, similar regression ...


I find this quite bizarre / puzzling, because this is a read-only
workload, with absolutely no writes, and tiny data set (~15MB), i.e.
everything fits into shared buffers. Why would that be affected by
checksums at all?

I spent some time profiling this, without much success. This is what I
get from perf top:

 Samples: 6M of event 'cycles:P', 4000 Hz, Event count (approx.):
5270683795302 lost: 0/0 drop: 0/0
 Overhead  Shared Object         Symbol
   50.94%  postgres              [.] pg_atomic_read_u32_impl
   17.32%  postgres              [.] pg_atomic_compare_exchange_u32_impl
   10.17%  postgres              [.] spin_delay
    5.83%  postgres              [.] pg_atomic_fetch_or_u32_impl
    1.64%  postgres              [.] pg_atomic_compare_exchange_u32_impl
    1.20%  postgres              [.] BufferDescriptorGetBuffer
    0.92%  postgres              [.] perform_spin_delay

and the report with backtraces says most of the time is spent here:

  --97.00%--btgettuple
        |
         --96.98%--_bt_first
              |
              |--48.82%--_bt_readfirstpage
              |     |
              |     |--44.57%--_bt_steppage
              |     |     |
              |     |      --44.45%--ReleaseBuffer
              |     |            |
              |     |             --44.43%--UnpinBuffer
              |     |                       |
              |     |                        ...
              |     |...
              |
               --48.11%--_bt_search
                    |

                      --47.89%--_bt_relandgetbuf



The atomic ops come from pinning/unpinning buffers. I realize it's
possible it gets much more expensive under concurrency (the clients
simply have to compete when updating the same counter, and with enough
clients there'll be more conflicts and retries). Kinda unfortunate, and
maybe we should do something about it, not sure.

But why would it depend on checksums at all? This read-only test should
be entirely in-memory, so how come it's affected?


regards

-- 
Tomas Vondra

  2. Re: strange perf regression with data checksums

    Aleksander Alekseev <aleksander@timescale.com> — 2025-05-09T12:53:52Z

    Hi Tomas,

> While running some benchmarks comparing 17 and 18, I ran into a simple
> workload where 18 throughput drops by ~80%. After pulling my hair for a
> couple hours I realized the change that triggered this is 04bec894a04c,
> which set checksums on by default. Which is very bizarre, because the
> workload is read-only and fits into shared buffers.
>
> [...]
>
> But why would it depend on checksums at all? This read-only test should
> be entirely in-memory, so how come it's affected?

These are interesting results.

Just wanted to clarify: did you make sure that all the hint bits were
set before executing the benchmark?

I'm not claiming that hint bits are necessarily the reason for the
observed behavior but when something is off with presumably read-only
queries this is the first reason that comes to mind. At least we
should make sure hint bits are excluded from the equation. If memory
serves, VACUUM FULL and CHECKPOINT after filling the table and
creating the index should do the trick.

-- 
Best regards,
Aleksander Alekseev

  3. Re: strange perf regression with data checksums

    Tomas Vondra <tomas@vondra.me> — 2025-05-09T13:06:30Z

    On 5/9/25 14:53, Aleksander Alekseev wrote:
> Hi Tomas,
> 
>> While running some benchmarks comparing 17 and 18, I ran into a simple
>> workload where 18 throughput drops by ~80%. After pulling my hair for a
>> couple hours I realized the change that triggered this is 04bec894a04c,
>> which set checksums on by default. Which is very bizarre, because the
>> workload is read-only and fits into shared buffers.
>>
>> [...]
>>
>> But why would it depend on checksums at all? This read-only test should
>> be entirely in-memory, so how come it's affected?
> 
> These are interesting results.
> 
> Just wanted to clarify: did you make sure that all the hint bits were
> set before executing the benchmark?
> 
> I'm not claiming that hint bits are necessarily the reason for the
> observed behavior but when something is off with presumably read-only
> queries this is the first reason that comes to mind. At least we
> should make sure hint bits are excluded from the equation. If memory
> serves, VACUUM FULL and CHECKPOINT after filling the table and
> creating the index should do the trick.
> 

Good question. I haven't checked that explicitly, but it's a tiny data
set (15MB) and I observed this even on long benchmarks with tens of
millions of queries. So the hint bits should have been set.

Also, I should have mentioned the query does an index-only scan, and the
pin/unpin calls are on index pages, not on the heap.


regards

-- 
Tomas Vondra

  4. Re: strange perf regression with data checksums

    Aleksander Alekseev <aleksander@timescale.com> — 2025-05-09T13:19:35Z

    Hi,

> > I'm not claiming that hint bits are necessarily the reason for the
> > observed behavior but when something is off with presumably read-only
> > queries this is the first reason that comes to mind. At least we
> > should make sure hint bits are excluded from the equation. If memory
> > serves, VACUUM FULL and CHECKPOINT after filling the table and
> > creating the index should do the trick.
>
> Good question. I haven't checked that explicitly, but it's a tiny data
> set (15MB) and I observed this even on long benchmarks with tens of
> millions of queries. So the hint bits should have been set.
>
> Also, I should have mentioned the query does an index-only scan, and the
> pin/unpin calls are on index pages, not on the heap.

There is one more thing I would check. As I recall perf shows only
on-CPU time while actually the backends may be sleeping on the locks
most of the time. If this is the case perf will not show you the
accurate picture.

In order to check this personally I create gdb.script with a single GDB command:

```
bt
```

And execute:

```
gdb --batch --command=gdb.script -p (backend_pid_here)
```

... 10+ times or so. If what you are observing is actually a lock
contention and the backend sleeps on a lock most of the time, 8/10 or
so stacktraces will show you this.

I assume of course that the benchmark is done on release builds with
disabled Asserts, etc.

BTW do you believe this is a problem related exclusively to NUMA CPUs
with 90+ cores or I can reproduce it on SMT as well?

-- 
Best regards,
Aleksander Alekseev

  5. Re: strange perf regression with data checksums

    Tomas Vondra <tomas@vondra.me> — 2025-05-09T13:59:59Z

    On 5/9/25 15:19, Aleksander Alekseev wrote:
> Hi,
> 
>>> I'm not claiming that hint bits are necessarily the reason for the
>>> observed behavior but when something is off with presumably read-only
>>> queries this is the first reason that comes to mind. At least we
>>> should make sure hint bits are excluded from the equation. If memory
>>> serves, VACUUM FULL and CHECKPOINT after filling the table and
>>> creating the index should do the trick.
>>
>> Good question. I haven't checked that explicitly, but it's a tiny data
>> set (15MB) and I observed this even on long benchmarks with tens of
>> millions of queries. So the hint bits should have been set.
>>
>> Also, I should have mentioned the query does an index-only scan, and the
>> pin/unpin calls are on index pages, not on the heap.
> 
> There is one more thing I would check. As I recall perf shows only
> on-CPU time while actually the backends may be sleeping on the locks
> most of the time. If this is the case perf will not show you the
> accurate picture.
> 

Right, perf only shows on-cpu time (at least by default). But the
backends really are consuming 100% CPU (or close to that, the pgbench
needs some CPU too), there are no lock waits that I can see.

I forgot to share flamegraphs I collected on the EPYC machine, for cases
with 96 clients. So here they are.

> In order to check this personally I create gdb.script with a single GDB command:
> 
> ```
> bt
> ```
> 
> And execute:
> 
> ```
> gdb --batch --command=gdb.script -p (backend_pid_here)
> ```
> 
> ... 10+ times or so. If what you are observing is actually a lock
> contention and the backend sleeps on a lock most of the time, 8/10 or
> so stacktraces will show you this.
> 
> I assume of course that the benchmark is done on release builds with
> disabled Asserts, etc.

Yes, there are regular builds with just --enable-debug --enable-depend,
nothing else (certainly not asserts).


> 
> BTW do you believe this is a problem related exclusively to NUMA CPUs
> with 90+ cores or I can reproduce it on SMT as well?
> 

No idea. I couldn't reproduce this on my ryzen machine at all, but that
only has 12 cores. The xeon (with 2x22 cores) seems to reproduce it, but
the difference is much smaller (1.2M vs. 1.5M tps), the pin/unpin have
only ~5% CPU, not 50%. Assuming it's the same issue, of course.


regards

-- 
Tomas Vondra

  6. Re: strange perf regression with data checksums

    Tomas Vondra <tomas@vondra.me> — 2025-05-19T16:13:02Z

    Hi,

I looked at this again, and I think the reason is mostly obvious. Both
why it's trashing, and why it happens with checksums=on ...

The reason why it happens is that PinBuffer does this:

    old_buf_state = pg_atomic_read_u32(&buf->state);
    for (;;)
    {
        if (old_buf_state & BM_LOCKED)
            old_buf_state = WaitBufHdrUnlocked(buf);

        buf_state = old_buf_state;

        ... modify state ...

        if (pg_atomic_compare_exchange_u32(&buf->state, &old_buf_state,
                           buf_state))
        {
        ...
        break;
        }
    }

So, we read the buffer state (which is where pins are tracked), possibly
waiting for it to get unlocked. Then we modify the state, and update it,
but only if it didn't change. If it did change, we retry.

Of course, as the number of sessions grows, the probability of something
updating the state in between increases. Another session might have
pinned the buffer, for example. This causes retries.

I added a couple counters to track how many loops are needed, and with
96 clients this needs about 800k retries per 100k calls, so about 8
retries per call. With 32 clients, this needs only about 25k retries, so
0.25 retry / call. That's a huge difference.

I believe enabling data checksums simply makes it more severe, because
the BufferGetLSNAtomic() has to obtain header lock, which uses the same
"state" field, with exactly the same retry logic. It can probably happen
even without it, but as the lock is exclusive, it also "serializes" the
access, making the conflicts more likely.

BufferGetLSNAtomic does this:

    bufHdr = GetBufferDescriptor(buffer - 1);
    buf_state = LockBufHdr(bufHdr);
    lsn = PageGetLSN(page);
    UnlockBufHdr(bufHdr, buf_state);

AFAICS the lock is needed simply to read a consistent value from the
page header, but maybe we could have an atomic variable with a copy of
the LSN in the buffer descriptor?


regards

-- 
Tomas Vondra

  7. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-05-19T16:42:24Z

    On Fri, May 9, 2025 at 9:06 AM Tomas Vondra <tomas@vondra.me> wrote:
> Good question. I haven't checked that explicitly, but it's a tiny data
> set (15MB) and I observed this even on long benchmarks with tens of
> millions of queries. So the hint bits should have been set.
>
> Also, I should have mentioned the query does an index-only scan, and the
> pin/unpin calls are on index pages, not on the heap.

We don't actually need to call BufferGetLSNAtomic() from _bt_readpage
during index-only scans (nor during bitmap index scans). We can just
not call BufferGetLSNAtomic() at all (except during plain index
scans), with no possible downside.

In general, we call BufferGetLSNAtomic() to stash the page LSN within
so->currPos.lsn, for later. so->currPos.lsn provides us with a way to
detect whether the page was modified during the period in which we
dropped our pin on the leaf page -- plain index scans cannot set
LP_DEAD bits on dead index tuples within _bt_killitems() if the page
has changed. But, index-only scans never drop the pin on the leaf page
to begin with, and so don't even use so->currPos.lsn (bitmap index
scans *never* call _bt_killitems(), and so obviously have no possible
use for so->currPos.lsn, either).

-- 
Peter Geoghegan

  8. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-05-19T17:20:33Z

    On Mon, May 19, 2025 at 12:42 PM Peter Geoghegan <pg@bowt.ie> wrote:
> We don't actually need to call BufferGetLSNAtomic() from _bt_readpage
> during index-only scans (nor during bitmap index scans). We can just
> not call BufferGetLSNAtomic() at all (except during plain index
> scans), with no possible downside.

Attached quick-and-dirty prototype patch shows how this could work. It
fully avoids calling BufferGetLSNAtomic() from _bt_readpage() during
index-only scans. I find that "meson test" passes with the patch
applied (I'm reasonably confident that this general approach is
correct).

Does this patch of mine actually fix the regressions that you're
concerned about?

-- 
Peter Geoghegan

  9. Re: strange perf regression with data checksums

    Tomas Vondra <tomas@vondra.me> — 2025-05-19T18:01:30Z

    
On 5/19/25 19:20, Peter Geoghegan wrote:
> On Mon, May 19, 2025 at 12:42 PM Peter Geoghegan <pg@bowt.ie> wrote:
>> We don't actually need to call BufferGetLSNAtomic() from _bt_readpage
>> during index-only scans (nor during bitmap index scans). We can just
>> not call BufferGetLSNAtomic() at all (except during plain index
>> scans), with no possible downside.
> 
> Attached quick-and-dirty prototype patch shows how this could work. It
> fully avoids calling BufferGetLSNAtomic() from _bt_readpage() during
> index-only scans. I find that "meson test" passes with the patch
> applied (I'm reasonably confident that this general approach is
> correct).
> 
> Does this patch of mine actually fix the regressions that you're
> concerned about?
> 

For index-only scans, yes. The numbers I used to see were

  64 clients: 1.2M tps
  96 clients: 0.6M tps

and with the patch it's

  64 clients: 1.2M tps
  96 clients: 2.9M tps

I'm aware it more than doubles, even if the client count grew only 1.5x.
I believe this is due to a VM config issue I saw earlier, but it wasn't
fixed on this particular VM (unfortunate, but out of my control).

The regular index scan however still have this issue, although it's not
as visible as for IOS. If I disable IOS, the throughput is

  64 clients: 0.7M tps
  96 clients: 0.6M tps

And the profile looks like this:

Overhead  Shared Object         Symbol
  28.85%  postgres              [.] PinBuffer
  22.61%  postgres              [.] UnpinBufferNoOwner
   9.67%  postgres              [.] BufferGetLSNAtomic
   4.92%  [kernel]              [k] finish_task_switch.isra.0
   3.78%  [kernel]              [k] _raw_spin_unlock_irqrestore
   ...

In an earlier message I mentioned maybe we could add an atomic variable
tracking the page LSN, so that we don't have to obtain the header lock.
I didn't have time to try yet.


regards

-- 
Tomas Vondra

  10. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-05-19T18:19:30Z

    On Mon, May 19, 2025 at 2:01 PM Tomas Vondra <tomas@vondra.me> wrote:
> For index-only scans, yes.

Great.

> The regular index scan however still have this issue, although it's not
> as visible as for IOS.

We can do somewhat better with plain index scans than my initial v1
prototype, without any major difficulties. There's more low-hanging
fruit.

We could also move the call to BufferGetLSNAtomic (that currently
takes place inside _bt_readpage) over to _bt_drop_lock_and_maybe_pin.
That way we'd only need to call BufferGetLSNAtomic for those leaf
pages that will actually need to have some index tuples returned to
the scan (through the btgettuple interface). In other words, we only
need to call BufferGetLSNAtomic for pages that _bt_readpage returns
"true" for when called. There are plenty of leaf pages that
_bt_readpage will return "false" for, especially during large range
scans, and skip scans.

It's easy to see why this extension to my v1 POC is correct: the whole
point of dropping the leaf page pin is that we don't block VACUUM when
btgettuple returns -- but btgettuple isn't going to return until the
next call to _bt_readpage that returns "true" actually takes place (or
until the whole scan ends).

-- 
Peter Geoghegan

  11. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-05-19T18:44:16Z

    On Mon, May 19, 2025 at 2:19 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Mon, May 19, 2025 at 2:01 PM Tomas Vondra <tomas@vondra.me> wrote:
> > The regular index scan however still have this issue, although it's not
> > as visible as for IOS.
>
> We can do somewhat better with plain index scans than my initial v1
> prototype, without any major difficulties. There's more low-hanging
> fruit.
>
> We could also move the call to BufferGetLSNAtomic (that currently
> takes place inside _bt_readpage) over to _bt_drop_lock_and_maybe_pin.
> That way we'd only need to call BufferGetLSNAtomic for those leaf
> pages that will actually need to have some index tuples returned to
> the scan (through the btgettuple interface).

Attached is v2, which does things this way. What do you think?

v2 also manages to avoid calling BufferGetLSNAtomic during all bitmap
index scans. You didn't complain about any regressions in bitmap index
scans, but I see no reason to take any chances (it's easy to just not
call BufferGetLSNAtomic there).

-- 
Peter Geoghegan

  12. Re: strange perf regression with data checksums

    Andres Freund <andres@anarazel.de> — 2025-05-19T19:37:14Z

    Hi,

On 2025-05-19 18:13:02 +0200, Tomas Vondra wrote:
> I believe enabling data checksums simply makes it more severe, because
> the BufferGetLSNAtomic() has to obtain header lock, which uses the same
> "state" field, with exactly the same retry logic. It can probably happen
> even without it, but as the lock is exclusive, it also "serializes" the
> access, making the conflicts more likely.
>
> BufferGetLSNAtomic does this:
>
>     bufHdr = GetBufferDescriptor(buffer - 1);
>     buf_state = LockBufHdr(bufHdr);
>     lsn = PageGetLSN(page);
>     UnlockBufHdr(bufHdr, buf_state);
>
> AFAICS the lock is needed simply to read a consistent value from the
> page header, but maybe we could have an atomic variable with a copy of
> the LSN in the buffer descriptor?

I think we can do better - something like

#ifdef PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY
    lsn = PageGetLSN(page);
#else
    buf_state = LockBufHdr(bufHdr);
    lsn = PageGetLSN(page);
    UnlockBufHdr(bufHdr, buf_state);
#endif

All perf relevant systems support reading 8 bytes without a chance of
tearing...

Greetings,

Andres Freund

  13. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-05-19T19:45:01Z

    On Mon, May 19, 2025 at 3:37 PM Andres Freund <andres@anarazel.de> wrote:
> I think we can do better - something like
>
> #ifdef PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY
>     lsn = PageGetLSN(page);
> #else
>     buf_state = LockBufHdr(bufHdr);
>     lsn = PageGetLSN(page);
>     UnlockBufHdr(bufHdr, buf_state);
> #endif
>
> All perf relevant systems support reading 8 bytes without a chance of
> tearing...

Even assuming that this scheme works perfectly, I'd still like to
pursue the idea I had about fixing this in nbtree.

The relevant nbtree code seems more elegant if we avoid calling
BufferGetLSNAtomic() unless and until its return value might actually
be useful. It's quite a lot easier to understand the true purpose of
so->currPos.lsn with this new structure.

--
Peter Geoghegan

  14. Re: strange perf regression with data checksums

    Tomas Vondra <tomas@vondra.me> — 2025-05-19T20:17:16Z

    On 5/19/25 20:44, Peter Geoghegan wrote:
> On Mon, May 19, 2025 at 2:19 PM Peter Geoghegan <pg@bowt.ie> wrote:
>> On Mon, May 19, 2025 at 2:01 PM Tomas Vondra <tomas@vondra.me> wrote:
>>> The regular index scan however still have this issue, although it's not
>>> as visible as for IOS.
>>
>> We can do somewhat better with plain index scans than my initial v1
>> prototype, without any major difficulties. There's more low-hanging
>> fruit.
>>
>> We could also move the call to BufferGetLSNAtomic (that currently
>> takes place inside _bt_readpage) over to _bt_drop_lock_and_maybe_pin.
>> That way we'd only need to call BufferGetLSNAtomic for those leaf
>> pages that will actually need to have some index tuples returned to
>> the scan (through the btgettuple interface).
> 
> Attached is v2, which does things this way. What do you think?
> 
> v2 also manages to avoid calling BufferGetLSNAtomic during all bitmap
> index scans. You didn't complain about any regressions in bitmap index
> scans, but I see no reason to take any chances (it's easy to just not
> call BufferGetLSNAtomic there).
> 

Same effect as v1 for IOS, with regular index scans I see this:

64 clients: 0.7M tps
96 clients: 1.5M tps

So very similar improvement as for IOS.


regards

-- 
Tomas Vondra

  15. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-05-19T20:29:09Z

    On Mon, May 19, 2025 at 4:17 PM Tomas Vondra <tomas@vondra.me> wrote:
> Same effect as v1 for IOS, with regular index scans I see this:
>
> 64 clients: 0.7M tps
> 96 clients: 1.5M tps
>
> So very similar improvement as for IOS.

Just to be clear, you're saying my v2 appears to fix the problem
fully, for both plain index scans and index-only scans? (You haven't
mentioned bitmap index scans at all, I think, even though they are
relevant to v2.)

I'd be surprised if my v2 *fully* fixed the issue with plain index
scans. It's only going to avoid calls to BufferGetLSNAtomic()
following _bt_readpage calls that return false, but I doubt that your
particular pgbench-variant test queries really look like that.

*Looks again* Oh, wait. This is another one of those benchmarks with
an index scan that returns no rows whatsoever (just like on the other
recent thread involving regressions tied to the introduction of a new
skip support support function to nbtree). Fully makes sense that my v2
would fix that particular problem, even with plain index scans.
But...what about slightly different queries, that actually do return
some rows? Those will look different.

-- 
Peter Geoghegan

  16. Re: strange perf regression with data checksums

    Tomas Vondra <tomas@vondra.me> — 2025-05-20T00:21:46Z

    On 5/19/25 22:29, Peter Geoghegan wrote:
> On Mon, May 19, 2025 at 4:17 PM Tomas Vondra <tomas@vondra.me> wrote:
>> Same effect as v1 for IOS, with regular index scans I see this:
>>
>> 64 clients: 0.7M tps
>> 96 clients: 1.5M tps
>>
>> So very similar improvement as for IOS.
> 
> Just to be clear, you're saying my v2 appears to fix the problem
> fully, for both plain index scans and index-only scans? (You haven't
> mentioned bitmap index scans at all, I think, even though they are
> relevant to v2.)

With v2 the regression disappears, both for index-only scans and regular
index scans. I haven't tried anything with bitmap scans - I hit the
regression mostly by accident, on a workload that does IOS only. I may
try constructing something with bitmap scans, but I didn't have time for
that right now.

> 
> I'd be surprised if my v2 *fully* fixed the issue with plain index
> scans. It's only going to avoid calls to BufferGetLSNAtomic()
> following _bt_readpage calls that return false, but I doubt that your
> particular pgbench-variant test queries really look like that.
> 


Not sure. My workload is pretty simple, and similar to what I used in
the other nbtree regression (with malloc overhead), except that it's not
using partitioning.

pgbench -i -s 1 test
psql test <<EOF
update pgbench_accounts set bid = aid;
create index on pgbench_accounts(bid);
vacuum full;
analyze;
EOF

# select.sql
set enable_indexonlyscan = off;
select count(*) from pgbench_accounts where bid = 1

I don't know what "fully fix" means in this context. I see a massive
improvement with v2, I have no idea if that's the best we could do.

> *Looks again* Oh, wait. This is another one of those benchmarks with
> an index scan that returns no rows whatsoever (just like on the other
> recent thread involving regressions tied to the introduction of a new
> skip support support function to nbtree). Fully makes sense that my v2
> would fix that particular problem, even with plain index scans.
> But...what about slightly different queries, that actually do return
> some rows? Those will look different.
> 

Actually, this particular query does return rows (one, to be precise).

But you're right - it seems sensitive to how many rows are returned, and
at some point the contention goes away and there's no regression.

I need to do proper automated testing, to get reliable results. I've
been doing manual testing, but it's easy to make mistakes that way.

Do you have any suggestions what cases you'd like me to test?


regards

-- 
Tomas Vondra

  17. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-05-20T12:43:16Z

    On Mon, May 19, 2025 at 8:21 PM Tomas Vondra <tomas@vondra.me> wrote:
> With v2 the regression disappears, both for index-only scans and regular
> index scans. I haven't tried anything with bitmap scans - I hit the
> regression mostly by accident, on a workload that does IOS only. I may
> try constructing something with bitmap scans, but I didn't have time for
> that right now.

I imagine bitmap index scans will be similar to plain index scans.

> I don't know what "fully fix" means in this context. I see a massive
> improvement with v2, I have no idea if that's the best we could do.

You expected there to be *zero* performance impact from enabling
checksums for this workload, since it is a pure read-only workload.
That's what I meant by "fully fix".

> But you're right - it seems sensitive to how many rows are returned, and
> at some point the contention goes away and there's no regression.
>
> I need to do proper automated testing, to get reliable results. I've
> been doing manual testing, but it's easy to make mistakes that way.
>
> Do you have any suggestions what cases you'd like me to test?

Nothing comes to mind. Again, just be aware that we can only
completely avoid calling BufferGetLSNAtomic is only possible when:

* Scan is an index-only scan

OR

* Scan is a bitmap index scan

OR

* Scan is a plain index scan, reading a page that _bt_readpage()
returned "false" for when called.

In other words, plain index scans that read a lot of tuples might
receive no benefit whatsoever. It's possible that it already matters
less there anyway. It's also possible that there is some unforeseen
benefit from merely *delaying* the call to BufferGetLSNAtomic. But in
all likelihood these "unfixed" plain index scans are just as fast with
v2 as they are when run on master/baseline.


-- 
Peter Geoghegan

  18. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-05-20T12:46:25Z

    On Tue, May 20, 2025 at 8:43 AM Peter Geoghegan <pg@bowt.ie> wrote:
> I imagine bitmap index scans will be similar to plain index scans.

To be clear, I meant that the magnitude of the problem will be
similar. I do not mean that they aren't fixed by my v2. They should be
fully fixed by v2.

-- 
Peter Geoghegan

  19. Re: strange perf regression with data checksums

    Andres Freund <andres@anarazel.de> — 2025-05-21T16:29:11Z

    Hi,

On 2025-05-19 15:45:01 -0400, Peter Geoghegan wrote:
> On Mon, May 19, 2025 at 3:37 PM Andres Freund <andres@anarazel.de> wrote:
> > I think we can do better - something like
> >
> > #ifdef PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY
> >     lsn = PageGetLSN(page);
> > #else
> >     buf_state = LockBufHdr(bufHdr);
> >     lsn = PageGetLSN(page);
> >     UnlockBufHdr(bufHdr, buf_state);
> > #endif
> >
> > All perf relevant systems support reading 8 bytes without a chance of
> > tearing...
> 
> Even assuming that this scheme works perfectly, I'd still like to
> pursue the idea I had about fixing this in nbtree.
> 
> The relevant nbtree code seems more elegant if we avoid calling
> BufferGetLSNAtomic() unless and until its return value might actually
> be useful. It's quite a lot easier to understand the true purpose of
> so->currPos.lsn with this new structure.

I'm not against that - ISTM we should do both.

Greetings,

Andres Freund

  20. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-05-21T21:11:56Z

    On Wed, May 21, 2025 at 12:29 PM Andres Freund <andres@anarazel.de> wrote:
> > The relevant nbtree code seems more elegant if we avoid calling
> > BufferGetLSNAtomic() unless and until its return value might actually
> > be useful. It's quite a lot easier to understand the true purpose of
> > so->currPos.lsn with this new structure.
>
> I'm not against that - ISTM we should do both.

Agreed.

Long term, we should move all of this stuff out of index AMs, which
don't have any good reason for doing their own thing. In a sense, the
known bugs in GiST and SP-GiST index-only scans (the "must not drop
the index page pin during index-only scans to avoid unsafe concurrent
TID recycling" bugs) exist because those index AMs couldn't just opt
in to some generic scheme, used by all index AMs that support
amgettuple-based scans.

Technically, the LSN is only needed to make kill_prior_tuple LP_DEAD
bit setting safe when the page pin is dropped. But it still seems
related to those GiST + SP-GiST IOS bugs, since we also need to
consider when dropping the pin is categorically unsafe (only nbtree
gets that aspect right currently).

-- 
Peter Geoghegan

  21. Re: strange perf regression with data checksums

    Tomas Vondra <tomas@vondra.me> — 2025-05-22T12:56:33Z

    Hi,

I finally had time to do more rigorous testing on the v1/v2 patches.
Attached is a .tgz with test script that initializes a pgbench scale 1,
and then:

  * Modifies the data to have different patterns / number of matching
    rows, etc. This is dobe by scripts in init/ directory.

  * Runs queries that either match or do not match any rows. This is
    done by scripts in select/ directory.

  * 32, 64 and 96 clients (the system has ~96 cores)

The scripts also force a particular scan type (bitmap/index/index-only),
and may also pin the processes to CPUs in different ways:

  * default = no pinning, it's up to scheduler
  * colocated = pgbench/backend always on the same core
  * random = pgbench/backend always on a different random core

This is done by a custom pgbench patch (can share, if needed). I found
the pinning may have *massive* impact in some cases.

There's also CSV with raw results, and two PDF files with a summary of
the results:

  * results-relative-speedup-vs-master.pdf - Shows throughput relative
    to master (for the same client count), 100% means no difference.

  * results-relative-speedup-vs-32.pdf - Slightly different view on the
    data, showing "scalability" for a given build. It compares
    throughput to "expected" multiple of the result we got for 32
    clients. 100% means linear scalability.

As usual, green=good, red=bad. My observation is that v2 performs better
than v1 (more green, darker green). v2 helps even in cases where v1 did
not make any difference (e.g. some of the "nomatch" cases).

It's also interesting how much impact the pinnig has - the "colocated"
results are much better. It's also interesting that in a couple cases we
scale superlinearly, i.e. 96 has better throughput than 3x that of 32
clients.

I've seen this before, and I believe it's due to behavior of the
hardware, and some kernel optimizations. Perhaps there's something we
could learn from this, not sure.

Anyway, as a comparison of v1 and v2 I think this is enough.


regards

-- 
Tomas Vondra

  22. Re: strange perf regression with data checksums

    Tomas Vondra <tomas@vondra.me> — 2025-06-04T11:33:06Z

    Hi,

We had a RMT meeting yesterday, and we briefly discussed whether the v2
patch should go into PG18. And we concluded we probably should try to do
that.

On the one hand, it's not a PG18-specific bug, in the sense that older
releases are affected the same way. But it requires data checksums to be
enabled, and a lot of systems runs with checksums=off, simply because
that's the default. But 18 will (likely) change that.

We didn't get any complaints about this (at least I'm not aware of any),
but I suppose that's simply because people didn't have a comparison, and
treated it as "normal". But with the default changes, it'll be easier to
spot once they upgrade to PG18.

So better to get this in now, otherwise we may have to wait until PG19,
because of ABI (the patch adds a field into BTScanPosData, but maybe
it'd be possible to add it into padding, not sure).


regards

-- 
Tomas Vondra

  23. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-06-04T14:21:22Z

    On Wed, Jun 4, 2025 at 7:33 AM Tomas Vondra <tomas@vondra.me> wrote:
> So better to get this in now, otherwise we may have to wait until PG19,
> because of ABI (the patch adds a field into BTScanPosData, but maybe
> it'd be possible to add it into padding, not sure).

I agree. I can get this into shape for commit today.

Does anybody object to my proceeding with committing the patch on the
master branch/putting it in Postgres 18?  (FWIW I could probably fit
the new field into some BTScanPosData alignment padding, but I don't
favor back patching.)

I consider my patch to be low risk. There's a kind of symmetry to how
things work with the patch in place, which IMV makes things simpler.

--
Peter Geoghegan

  24. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-06-04T17:39:40Z

    On Wed, Jun 4, 2025 at 10:21 AM Peter Geoghegan <pg@bowt.ie> wrote:
> On Wed, Jun 4, 2025 at 7:33 AM Tomas Vondra <tomas@vondra.me> wrote:
> > So better to get this in now, otherwise we may have to wait until PG19,
> > because of ABI (the patch adds a field into BTScanPosData, but maybe
> > it'd be possible to add it into padding, not sure).
>
> I agree. I can get this into shape for commit today.

Attached is v3, which is functionally the same as v2. It has improved
comments, and a couple of extra assertions. Plus there's a real commit
message now.

I also relocated the code that sets so.drop_pin from
_bt_preprocess_keys to btrescan. That seems like the more logical
place for it.

My current plan is to commit this in the next couple of days.

-- 
Peter Geoghegan

  25. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-06-06T16:33:44Z

    On Wed, Jun 4, 2025 at 1:39 PM Peter Geoghegan <pg@bowt.ie> wrote:
> My current plan is to commit this in the next couple of days.

Pushed.

It would be great if we could also teach BufferGetLSNAtomic() to just
call PageGetLSN() (at least on PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY
platforms), but my sense is that that's not going to happen in
Postgres 18.

-- 
Peter Geoghegan

  26. Re: strange perf regression with data checksums

    Alexander Law <exclusion@gmail.com> — 2025-06-09T06:00:00Z

    Hello Peter,

06.06.2025 19:33, Peter Geoghegan wrote:
> On Wed, Jun 4, 2025 at 1:39 PM Peter Geoghegan<pg@bowt.ie> wrote:
>> My current plan is to commit this in the next couple of days.
> Pushed.

Please look at the following script, which falsifies an assertion
introduced with e6eed40e4:
create user u;
grant all on schema public to u;
set session authorization u;
create table tbl1 (a int);
create function f1() returns int language plpgsql as $$ invalid $$;

select sum(1) over (partition by 1 order by objid)
   from pg_shdepend
   left join pg_stat_sys_indexes on refclassid = relid
   left join tbl1 on true
limit 1;
(I've simplified an assert-triggering query generated by SQLsmith.)

TRAP: failed Assert("!BTScanPosIsPinned(so->currPos)"), File: "nbtutils.c", Line: 3379, PID: 1621028
ExceptionalCondition at assert.c:52:13
_bt_killitems at nbtutils.c:3380:3
_bt_steppage at nbtsearch.c:2134:5
_bt_next at nbtsearch.c:1560:7
btgettuple at nbtree.c:276:6
index_getnext_tid at indexam.c:640:25
index_getnext_slot at indexam.c:729:10
IndexNext at nodeIndexscan.c:131:9
ExecScanFetch at execScan.h:126:10
...

Best regards,
Alexander

  27. Re: strange perf regression with data checksums

    Mihail Nikalayeu <mihailnikalayeu@gmail.com> — 2025-06-09T12:47:18Z

    Hello, Peter!

>
> I also relocated the code that sets so.drop_pin from
> _bt_preprocess_keys to btrescan. That seems like the more logical
> place for it.
>

I was rebasing [0] and noticed dropPin is not initialized in
btbeginscan, which seems to be suspicious for me.
Is it intended?

[0]: https://commitfest.postgresql.org/patch/5151/

  28. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-06-09T14:47:35Z

    Hi Alexander,

On Mon, Jun 9, 2025 at 2:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> Please look at the following script, which falsifies an assertion

> TRAP: failed Assert("!BTScanPosIsPinned(so->currPos)"), File: "nbtutils.c", Line: 3379, PID: 1621028

I can reproduce this. I think that it's an old bug.

The problem is related to mark/restore (used by merge joins), where
nbtree can sometimes independently hold a second pin on leaf pages
(albeit very briefly, inside _bt_steppage). The cause of the failure
of my recently-added assertion is that it assumes that so->dropPin
always implies !BTScanPosIsPinned(so->currPos) at the start of
_bt_killitems. I don't think that that assumption is unreasonable,
even in light of this assertion problem.

I think that the real problem is the way that _bt_killitems releases
resources. _bt_killitems doesn't try to leave so->currPos as it was at
the start of its call -- this is nothing new. The overall effect is
that during so->dropPin _bt_steppage calls, we *generally* don't call
IncrBufferRefCount in the "bump pin on current buffer for assignment
to mark buffer" path, but *will* call IncrBufferRefCount when we
happened to need to call _bt_killitems. Calling _bt_killitems
shouldn't have these side-effects.

I can make the assertion failure go away by teaching _bt_killitems to
leave so->currPos in the same state that it was in when _bt_killitems
was called. If there was no pin on the page when _bt_killitems was
called, then there should be no pin held when it returns. See the
attached patch.

As I said, I think that this is actually an old bug. After all,
_bt_killitems is required to test so->currPos.lsn against the same
page's current LSN if the page pin was dropped since _bt_readpage was
first called -- regardless of any other details. I don't think that
it'll consistently do that in this hard-to-test mark/restore path on
any Postgres version, so there might be a risk of failing to detect an
unsafe concurrent TID recycling hazard.

I've always thought that the whole idiom of testing
BTScanPosIsPinned() in a bunch of places was very brittle. I wonder if
it makes sense to totally replace those calls/tests with similar tests
of the new so->dropPin field.

-- 
Peter Geoghegan

  29. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-06-09T14:49:27Z

    On Mon, Jun 9, 2025 at 8:48 AM Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:
> I was rebasing [0] and noticed dropPin is not initialized in
> btbeginscan, which seems to be suspicious for me.
> Is it intended?

That's pretty normal. We don't have access to the scan descriptor
within btbeginscan.

This is also why we do things like allocate so->currTuples within
btrescan. We don't yet know if the scan will be an index-only scan
when btbeginscan is called.

-- 
Peter Geoghegan

  30. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-06-09T19:04:43Z

    On Mon, Jun 9, 2025 at 10:47 AM Peter Geoghegan <pg@bowt.ie> wrote:
> I've always thought that the whole idiom of testing
> BTScanPosIsPinned() in a bunch of places was very brittle. I wonder if
> it makes sense to totally replace those calls/tests with similar tests
> of the new so->dropPin field.

It's definitely possible to avoid testing BTScanPosIsPinned like this
entirely. Attached v2 patch shows how this is possible.

v2 fully removes BTScanPosUnpinIfPinned, and limits the use of
BTScanPosIsPinned to assertions. I think that this approach makes our
buffer pin resource management strategy much clearer, particularly in
cases involving mark/restore. This might help with Tomas Vondra's
index prefetching patch -- I know that Tomas found the mark/restore
aspects of his latest approach challenging.

In v2, I added an assertion at the top of _bt_steppage that
independently verify the same conditions to the ones that are already
tested whenever _bt_steppage calls _bt_killitems (these include the
assertion that Alexander showed could fail):

static bool
_bt_steppage(IndexScanDesc scan, ScanDirection dir)
{
    BTScanOpaque so = (BTScanOpaque) scan->opaque;
    BlockNumber blkno,
                lastcurrblkno;

    Assert(BTScanPosIsValid(so->currPos));
    if (!so->dropPin)
        Assert(BTScanPosIsPinned(so->currPos));
    else
        Assert(!BTScanPosIsPinned(so->currPos));
...

That way if there are remaining issues like the ones that Alexander
reported, we don't rely on reaching _bt_killitems to get an assertion
failure.

(Note that adding these assertions necessitated making _bt_firstpage
call _bt_readnextpage instead of its _bt_steppage wrapper.
_bt_steppage is now "Only called when _bt_drop_lock_and_maybe_pin was
called for so->currPos", which doesn't apply to the case where
_bt_firstpage previously called _bt_steppage. This brings
_bt_firstpage in line with _bt_readnextpage itself -- now we only call
_bt_steppage for a page that we returned to the scan via btgettuple/a
page that might have had its pin released by
_bt_drop_lock_and_maybe_pin.)

v2 also completely removes all IncrBufferRefCount() calls from nbtree.
These were never really necessary, since we don't actually need to
hold more than one pin at a time on the same page at any point in
cases involving mark/restore (nor any other case). We can always get
by with no more than one buffer pin on the same page.

There were 2 IncrBufferRefCount() calls total, both of which v2 removes:

The first IncrBufferRefCount call removed was in _bt_steppage. We copy
so->currPos into so->markPos when leaving the page in _bt_steppage.
We're invalidating so->currPos in passing here, so we don't *need* to
play games with IncrBufferRefCount -- we can just not release the
original pin, based on the understanding that so->markPos has the pin
"transferred" to (so->currPos can therefore be invalidated without
having its pin unpinned).

The second IncrBufferRefCount call removed was in btrestrpos. When we
restore a mark, we need to keep around the original mark, in case it
needs to be restored a little later -- the so->markPos mark must not
go away. But that in itself doesn't necessitate holding multiple
buffer pins on the same page at once. We can keep the old mark around,
without keeping a separate buffer pin for so->markPos. We can just set
so->markItemIndex instead -- that's an alternative (and more
efficient) way of representing the mark (the so->markPos
representation isn't truly needed). This is okay, since our new
so->currPos is for the same page as the mark -- that's how
so->markItemIndex  is supposed to be used.

-- 
Peter Geoghegan

  31. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-06-10T19:00:01Z

    On Mon, Jun 9, 2025 at 10:47 AM Peter Geoghegan <pg@bowt.ie> wrote:
> As I said, I think that this is actually an old bug. After all,
> _bt_killitems is required to test so->currPos.lsn against the same
> page's current LSN if the page pin was dropped since _bt_readpage was
> first called -- regardless of any other details. I don't think that
> it'll consistently do that in this hard-to-test mark/restore path on
> any Postgres version, so there might be a risk of failing to detect an
> unsafe concurrent TID recycling hazard.

I have confirmed that this flavor of the problem has existed for a
long time. We'll need to backpatch the fix to all supported branches.

Attached v3 breaks this part out into its own commit. v3 has a proper
commit message now, which explains the implications of the bug on
earlier releases. I won't repeat that here, except to say that it's
just about possible that kill_prior_tuple LP_DEAD bit setting will
incorrectly set LP_DEAD bits due to this bug.

I've been thinking about also committing the second patch against
master/Postgres 18 only -- but I've since had cold feet about that. It
is really just refactoring to make the code more robust against these
sorts of issues by conditioning everything on so->dropPin. Right now,
on 18, we're only doing that in _bt_killitems. That was enough to
detect this bug, but it might not work out that way in the future, if
there's a new (or an old) bug of the same general nature somewhere
else.

Current plan: Commit 0001 on all supported branches in the next couple
of days. Unless I hear objections.

I will wait until Postgres 18 branches from master before committing
0002. It's important refactoring work, but on reflection I find it
hard to justify not just waiting.

--
Peter Geoghegan

  32. Re: strange perf regression with data checksums

    Peter Geoghegan <pg@bowt.ie> — 2025-06-11T16:39:40Z

    On Tue, Jun 10, 2025 at 3:00 PM Peter Geoghegan <pg@bowt.ie> wrote:
> I have confirmed that this flavor of the problem has existed for a
> long time. We'll need to backpatch the fix to all supported branches.

> Current plan: Commit 0001 on all supported branches in the next couple
> of days. Unless I hear objections.

Pushed 0001 earlier today, backpatching to all supported releases.
Thanks for the report, Alexander!

I should acknowledge that, on further reflection, I have significant
doubts about whether it's actually possible for the bug to cause data
corruption on earlier releases (by incorrectly setting LP_DEAD bits).
The reasons for my doubts are complicated (much more complicated than
the fix itself). I think that backpatching was still the right call --
I just don't want to be needlessly alarmist here.

When I ran Alexander's test case on an earlier version of Postgres
(anything before my recent commit e6eed40e), I did indeed observe that
there were 2 _bt_killitems calls, the first of which behaved like what
we now call a !so->dropPin scan, and the second of which (against the
same page/scan position following restore of a mark) behaved like a
so->dropPin scan. This was obviously never intended by the 2015 commit
where the problem originated (commit 2ed5b87f), and seems wrong on its
face. We're not doing what we're supposed to be doing during the
second _bt_killitems call, which I imagined opened up a window for
data corruption.

However, it has since occured to me that the first _bt_killitems is
only able to spuriously set so->curPoss.buf and confuse its
_bt_steppage *when it succeeded* in setting at least one LP_DEAD bit.
And in order to succeed, it will have to have correctly checked
so->currPos.lsn against the page's LSN at that time first. The LSN
can't have changed at that point -- if it did then there'd be no pin
set in so->currPos.buf (just because that's how that aspect was
handled), and so no confusion, and so no second confused call to
_bt_killitems later on.

The pin held between the first and second _bt_killitems calls *is*
sufficient as an interlock against VACUUM, I think, since we *also*
checked the LSN during the first _bt_killitems.

As Tom would say, it accidentally failed to fail. There is no reason
to allow it, especially given that it can be fully avoided by making
dropped-pin calls to _bt_killitems consistently release both their
lock and their pin (which, as I touched upon, is how _bt_killitems
always did it in cases where the page LSN changed since _bt_readpage
was called).

> I will wait until Postgres 18 branches from master before committing
> 0002. It's important refactoring work, but on reflection I find it
> hard to justify not just waiting.

I will start a new, dedicated thread for this.

-- 
Peter Geoghegan