Re: [PATCH] vacuum: stop using stream ring after failsafe

On Wed, Jul 22, 2026 at 12:53 PM Jingtang Zhang <mrdrivingduck(at)gmail(dot)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