[PATCH] vacuum: stop using stream ring after failsafe

On Wed, Jul 22, 2026 at 12:53 PM Jingtang Zhang <mrdrivingduck@gmail.com> wrote:

4830f1024325 made VACUUM's failsafe abandon its buffer access strategy
and use normal shared-buffer replacement. Commit 9256822608f3 later
converted VACUUM's first phase to use streaming read I/O.

Was it an oversight in the read stream conversion that the stream is
initialized with VACUUM's strategy, but is not updated when failsafe
clears vacrel->bstrategy? As a result, the active stream continues to
use the VACUUM ring for later heap reads.

Thanks for the report and patch. Yes, this was an oversight.

This patch allows a read stream's strategy to be changed for future I/O.
VACUUM switches the active stream to a NULL strategy when failsafe
activates, without modifying in-flight I/O operations.

Right, because it can change during an ongoing scan, there has to be
some way to set it for a new read after the strategy was deactivated.
And we have to make sure we retain information on the IOContext that
read operations were started in for accounting purposes.

For master, I actually think what we should do is save the IOContext
in the ReadBuffersOperation instead of the BufferAccessStrategy. I
think it is cleaner since you only need the IOContext when completing
the IO and you can pass the BufferAccessStrategy directly to
StartReadBuffers() without saving it in the ReadBuffersOperation.

And, when it comes to this patch and being able to "deactivate" the
buffer access strategy, we need a way to do so for future IOs without
affecting in-progress IOs, and this structure seems like the best way
to do that.

I've included two commits targeted at master. The first replaces
BufferAccessStrategy with IOContext in the ReadBuffersOperation and
the second resets the BufferAccessStrategy in the read stream to fix
the reported bug with failsafe mode. (attached and prefixed "master").

I'll note I did add an include of pgstat.h in bufmgr.h which is pretty
undesirable. I'm not sure if I should make a header for the
IOContext/IOObject stuff to avoid this.

For backbranches, we should just do what you did in your patch. I
included an updated version of it with a few tweaks. Luckily
ReadStream is opaque, so these changes are still ABI-compatible
(attached and prefixed "backbranches"). My proposed changes to master
would make ABI-breaking changes to ReadBuffersOperation, so we
shouldn't do that in backbranches.

- Melanie

On Wed, Jul 22, 2026 at 12:53 PM Jingtang Zhang <mrdrivingduck@gmail.com> wrote:

The stream calculates its pin limit and queue capacity when it is
created. It uses the original strategy. This patch deliberately changes
only buffer replacement for future reads; it does not resize the stream.

I just wanted to also address this point. It's true that when we stop
using the strategy in failsafe mode we will not adjust
max_pinned_buffers -- but we couldn't adjust that without the ability
to resize the buffers array in the read stream. Adding the ability to
change max_pinned_buffers during an ongoing read stream seems like
overkill to me. And making the array size bigger than the strategy
needs it to be in the off chance we will abandon the strategy also
feels wrong.

If we calculate the impact on readahead, with any non-toy
shared_buffers value and the default io_combine_limit and
maintenance_io_concurrency values, max_pinned_buffers is 272. With the
buffer access strategy, it is 128. That's a 2x reduction which is
non-negligible. However, I think the primary benefit of using shared
buffers instead of the strategy is that we don't have to write data
(and flush wal) to make space to read the next page to vacuum. The
reduced readahead distance isn't the end of the world -- and I don't
think the changes required to fix it are worth it.

- Melanie

On Mon, Aug 3, 2026 at 6:45 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:

For master, I actually think what we should do is save the IOContext
in the ReadBuffersOperation instead of the BufferAccessStrategy. I
think it is cleaner since you only need the IOContext when completing
the IO and you can pass the BufferAccessStrategy directly to
StartReadBuffers() without saving it in the ReadBuffersOperation.

And, when it comes to this patch and being able to "deactivate" the
buffer access strategy, we need a way to do so for future IOs without
affecting in-progress IOs, and this structure seems like the best way
to do that.

Andres pointed out off-list that we had seen regressions when adding
another parameter to StartReadBuffersImpl() because it currently has 6
arguments and the sysv-x86-64 ABI can only accommodate 6 arguments in
registers before spilling to stack. As such, most of my "elegant"
solutions don't work. One thing we could do is add another
StartReadBuffersImpl() flag for "use strategy". I tried that and
didn't love how it looked since we then have to check that flag before
using the strategy in 5 places and that can only grow in the future.

Instead, I propose we just take a small hit to the accounting and
simply clear the ReadBuffersOperations->strategy even of in-progress
IOs. It should only misattribute a bit of wait time -- and only for a
limited number of IOs once -- when we enter failsafe mode. It makes
for a simple, backpatchable solution. I've attached here what I intend
to commit later today.

- Melanie

On Thu, Aug 6, 2026 at 4:42 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:

Instead, I propose we just take a small hit to the accounting and
simply clear the ReadBuffersOperations->strategy even of in-progress
IOs. It should only misattribute a bit of wait time -- and only for a
limited number of IOs once -- when we enter failsafe mode. It makes
for a simple, backpatchable solution. I've attached here what I intend
to commit later today.

Okay, I pushed it, but right after pushing it I suddenly had the
thought, omg, it loops through all the ReadBuffersOperations and sets
them to NULL for _every_ page after entering failsafe mode. And we
obviously ought to fix that. Proposed patch to do that with a simple
local variable is attached. This time I'll let this one sit for longer
before pushing to avoid mistakes like last time.

- Melanie

Melanie Plageman <melanieplageman@gmail.com> writes:

Okay, I pushed it, but right after pushing it I suddenly had the
thought, omg, it loops through all the ReadBuffersOperations and sets
them to NULL for _every_ page after entering failsafe mode. And we
obviously ought to fix that. Proposed patch to do that with a simple
local variable is attached. This time I'll let this one sit for longer
before pushing to avoid mistakes like last time.

Don't wait too long, unless you want to revert 112c26838 et al
till after the upcoming releases. As things stand right now,
we'd ship what is in git.

regards, tom lane

On Thu, Aug 6, 2026 at 6:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Melanie Plageman <melanieplageman@gmail.com> writes:

Okay, I pushed it, but right after pushing it I suddenly had the
thought, omg, it loops through all the ReadBuffersOperations and sets
them to NULL for _every_ page after entering failsafe mode. And we
obviously ought to fix that. Proposed patch to do that with a simple
local variable is attached. This time I'll let this one sit for longer
before pushing to avoid mistakes like last time.

Don't wait too long, unless you want to revert 112c26838 et al
till after the upcoming releases. As things stand right now,
we'd ship what is in git.

roger that. I'm planning to push in the morning which will be before
Saturday's freeze.

- Melanie

On Thu, Aug 6, 2026 at 6:16 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:

On Thu, Aug 6, 2026 at 6:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Melanie Plageman <melanieplageman@gmail.com> writes:

Okay, I pushed it, but right after pushing it I suddenly had the
thought, omg, it loops through all the ReadBuffersOperations and sets
them to NULL for _every_ page after entering failsafe mode. And we
obviously ought to fix that. Proposed patch to do that with a simple
local variable is attached. This time I'll let this one sit for longer
before pushing to avoid mistakes like last time.

Don't wait too long, unless you want to revert 112c26838 et al
till after the upcoming releases. As things stand right now,
we'd ship what is in git.

roger that. I'm planning to push in the morning which will be before
Saturday's freeze.

Committed and backpatched.

- Melanie

Hi Melanie,

Thanks for taking a look. The committed fixes LGTM.

One question: are there general guidelines for keeping argument counts
low in hot PostgreSQL functions, or is this mainly assessed case by case?

---
Regards,
Jingtang

On Fri, Aug 7, 2026 at 11:20 AM Jingtang Zhang <mrdrivingduck@gmail.com> wrote:

One question: are there general guidelines for keeping argument counts
low in hot PostgreSQL functions, or is this mainly assessed case by case?

We don't worry about it unless it is the bottleneck in some workload
-- like if you have a workload where you can notice a performance
improvement by changing the number of arguments. The
StartReadBuffersImpl() thing came about when we committed the read
stream code and someone noticed regressions for fully cached
sequential scans as compared to without the read stream code. The read
stream adds its own overhead, so Andres/Thomas/Bilal and others
embarked on a journey to try to win back performance for that use case
by chipping away at various things guided by careful profiling and
benchmarking.

- Melanie