Horton

Thread

Commits

  1. Remove HeapBitmapScan's skip_fetch optimization

  1. TID recycling race during nbtree index-only scans that run on a standby

    Peter Geoghegan <pg@bowt.ie> — 2026-06-17T22:16:43Z

    Recently, I've been thinking a lot about the interlocking protocol
that prevents wrong answers during index-only scans, even with
concurrent TID recycling (since it is relevant to the index
prefetching work). I'm referring to the way index-only scans generally
hold a buffer pin on their scan's current leaf page position, which
will conflict with the cleanup locks that index vacuuming acquires on
every leaf page.

There are historical (and current) bugs that share the same basic
shape. There are live bugs in GiST and SP-GiST index only scans [1]. A
similar bug also affected bitmap scans, which previously used
information from the visibility map as an optimization (before we
fixed that bug) [2].

Unfortunately, there appears to be yet another bug of that general nature.

The bug in question affects nbtree index-only scans running during hot
standby; these scans can see "phantom" resurrected rows when VACUUM
recycles stub LP_DEAD line pointers in heap pages sooner than is safe.
The LP_DEAD stubs are needed as tombstones, but VACUUM can sometimes
win the race and mark them LP_UNUSED prematurely -- also setting the
relevant heap page all-visible in the VM. In other words, this bug's
general symptoms match those of the other bugs I mentioned.

This is only possible because the standby won't acquire cleanup locks
on *every* index page -- unlike during original execution. It will
only cleanup lock whatever index pages actually had one or more index
tuples removed during VACUUM, which isn't quite good enough. In other
words, the rationale for removing the "pin scan" logic in commits
f65b94f6, 3e4b7d87, and 687f2cd7 was subtly flawed in that it didn't
consider index-only scans, which are legitimately a special case.

Attached are 2 patches, both intended to show the general nature of the problem.

The first patch is a repro written by Claude code at my direction;
there are many tedious and fiddly details involved that aren't worth
discussing now. Multiple test cases show wrong answers, allowing the
bug to manifest in several different ways (delete+commit vs
insert+abort, page split vs index deletion).

The second patch resurrects the old "pin scan" logic into modern
nbtree, making the failing tests pass, and confirming my understanding
of the problem.

Neither patch is committable. The pin scan mechanism performed
terribly, and I cannot countenance actually bringing it back now. I
haven't yet given much thought to how we can fix this bug without
causing more harm than good. The rationale for removing the "pin scan"
logic was *almost* correct back in 2016; we simply failed to consider
how index-only scans are a special case (which, crucially, wasn't
documented anywhere in 2016, and still isn't today).

[1] https://postgr.es/m/CAH2-Wz=jjiNL9FCh8C1L-GUH15f4WFTWub2x+_NucngcDDcHKw@mail.gmail.com
[2] Fixed by April 2025 commit 459e7bf8
-- 
Peter Geoghegan

  2. Re: TID recycling race during nbtree index-only scans that run on a standby

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2026-06-22T11:38:07Z

    On Thu, 18 Jun 2026 at 00:17, Peter Geoghegan <pg@bowt.ie> wrote:
>
> Recently, I've been thinking a lot about the interlocking protocol
> that prevents wrong answers during index-only scans, even with
> concurrent TID recycling (since it is relevant to the index
> prefetching work). I'm referring to the way index-only scans generally
> hold a buffer pin on their scan's current leaf page position, which
> will conflict with the cleanup locks that index vacuuming acquires on
> every leaf page.

> The bug in question affects nbtree index-only scans running during hot
> standby; these scans can see "phantom" resurrected rows when VACUUM
> recycles stub LP_DEAD line pointers in heap pages sooner than is safe.
> The LP_DEAD stubs are needed as tombstones, but VACUUM can sometimes
> win the race and mark them LP_UNUSED prematurely -- also setting the
> relevant heap page all-visible in the VM.

All that, for queries active on that standby, right? This isn't an
issue about the primary, but rather one about WAL replay not currently
providing the right interlocks?

>  In other words, this bug's
> general symptoms match those of the other bugs I mentioned.
>
> This is only possible because the standby won't acquire cleanup locks
> on *every* index page -- unlike during original execution. It will
> only cleanup lock whatever index pages actually had one or more index
> tuples removed during VACUUM, which isn't quite good enough. In other
> words, the rationale for removing the "pin scan" logic in commits
> f65b94f6, 3e4b7d87, and 687f2cd7 was subtly flawed in that it didn't
> consider index-only scans, which are legitimately a special case.
>
> Attached are 2 patches, both intended to show the general nature of the problem.
>
> The first patch is a repro written by Claude code at my direction;
> there are many tedious and fiddly details involved that aren't worth
> discussing now. Multiple test cases show wrong answers, allowing the
> bug to manifest in several different ways (delete+commit vs
> insert+abort, page split vs index deletion).
>
> The second patch resurrects the old "pin scan" logic into modern
> nbtree, making the failing tests pass, and confirming my understanding
> of the problem.
>
> Neither patch is committable. The pin scan mechanism performed
> terribly, and I cannot countenance actually bringing it back now. I
> haven't yet given much thought to how we can fix this bug without
> causing more harm than good. The rationale for removing the "pin scan"
> logic was *almost* correct back in 2016; we simply failed to consider
> how index-only scans are a special case (which, crucially, wasn't
> documented anywhere in 2016, and still isn't today).

I have a few thoughts:

There are two bugs here, both arriving from the same issue of removing
dead items from the page without pin interlock:
1. page splits move cached dead items away from the page, allowing
them to be cleaned up from another page without the original page
(which may still have pins) being involved, and
2. replay of XLOG_BTREE_DELETE doesn't take a cleanup lock (which
provides the "pin interlock") on the page.

For the second case, obviously, we should use a cleanup lock for
XLOG_BTREE_DELETE.
For the first case, this probably also would be handled if we took a
cleanup lock on the original page during replay - the operations is
removing tuples from that page, and we don't yet know if they're going
to be recycled soon (though that is unlikely given page split's
efforts to avoid keeping dead tuples on the page, it is possible). See
attached (on top of your v1-0001).

Alternatively, we could hold normal read locks on the page on
replica's IOS while the scan needs that page. That'd risk getting
really bad performance in the normal case, though, where page
splits/cleanup happen much more rarely than insertions; while those
inserts also require interlocked access with page reads.


Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

PS. This once again reminds me, that with all the new buffer pin
-holding systems in AIO we probably should have a 'hurry up; I need to
access this buffer and you're blocking my very important job' signal
system.  Cursors (and other queries) that hold on to IOS' required
pins only because it decided VM checks should happen very lazily
should then be able to run those VM checks immediately on receiving
that signal, instead of having to wait for the query state to continue
on past the current page.

  3. Re: TID recycling race during nbtree index-only scans that run on a standby

    Peter Geoghegan <pg@bowt.ie> — 2026-07-07T18:32:51Z

    On Mon, Jun 22, 2026 at 7:38 AM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> On Thu, 18 Jun 2026 at 00:17, Peter Geoghegan <pg@bowt.ie> wrote:
> All that, for queries active on that standby, right? This isn't an
> issue about the primary, but rather one about WAL replay not currently
> providing the right interlocks?

Yes, it is. The old pin scan code made hot standby follow the same
locking protocol as the primary; we no longer do that, which leaves
these gaps.

> I have a few thoughts:
>
> There are two bugs here, both arriving from the same issue of removing
> dead items from the page without pin interlock:
> 1. page splits move cached dead items away from the page, allowing
> them to be cleaned up from another page without the original page
> (which may still have pins) being involved, and
> 2. replay of XLOG_BTREE_DELETE doesn't take a cleanup lock (which
> provides the "pin interlock") on the page.

FWIW I don't think defining this as two bugs is useful.

> For the second case, obviously, we should use a cleanup lock for
> XLOG_BTREE_DELETE.
> For the first case, this probably also would be handled if we took a
> cleanup lock on the original page during replay - the operations is
> removing tuples from that page, and we don't yet know if they're going
> to be recycled soon (though that is unlikely given page split's
> efforts to avoid keeping dead tuples on the page, it is possible). See
> attached (on top of your v1-0001).
>
> Alternatively, we could hold normal read locks on the page on
> replica's IOS while the scan needs that page. That'd risk getting
> really bad performance in the normal case, though, where page
> splits/cleanup happen much more rarely than insertions; while those
> inserts also require interlocked access with page reads.

Honestly, none of these options seem appealing. I don't have a better
idea, either.  :-(

> PS. This once again reminds me, that with all the new buffer pin
> -holding systems in AIO we probably should have a 'hurry up; I need to
> access this buffer and you're blocking my very important job' signal
> system.  Cursors (and other queries) that hold on to IOS' required
> pins only because it decided VM checks should happen very lazily
> should then be able to run those VM checks immediately on receiving
> that signal, instead of having to wait for the query state to continue
> on past the current page.

I've sometimes thought that a system like that could make sense for
aggressive VACUUMs that block indefinitely on a heap pin.

With the amgetbatch interface, this usually wouldn't make a difference
because the cursor scan would already be at the point where it eagerly
drops every index leaf page (and eagerly fills each page's/batch's
visibility info from the visibility map to make that safe). You'd have
to be quite unlucky for it to still happen.

-- 
Peter Geoghegan