Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode

Hi,

On 2023-04-04 13:53:15 -0400, Melanie Plageman wrote:
> > 8. I don't quite follow this comment:
> >
> > /*
> > * TODO: should this be 0 so that we are sure that vacuum() never
> > * allocates a new bstrategy for us, even if we pass in NULL for that
> > * parameter? maybe could change how failsafe NULLs out bstrategy if
> > * so?
> > */
> >
> > Can you explain under what circumstances would vacuum() allocate a
> > bstrategy when do_autovacuum() would not? Is this a case of a config
> > reload where someone changes vacuum_buffer_usage_limit from 0 to
> > something non-zero? If so, perhaps do_autovacuum() needs to detect
> > this and allocate a strategy rather than having vacuum() do it once
> > per table (wastefully).

Hm. I don't much like that we use a single strategy for multiple tables
today. That way even tiny tables never end up in shared_buffers. But that's
really a discussion for a different thread. However, if were to use a
per-table bstrategy, it'd be a lot easier to react to changes of the config.


I doubt it's worth adding complications to the code for changing the size of
the ringbuffer during an ongoing vacuum scan, at least for 16. Reacting to
enabling/disbling the ringbuffer alltogether seems a bit more important, but
still not crucial compared to making it configurable at all.

I think it'd be OK to add a comment saying something like "XXX: In the future
we might want to react to configuration changes of the ring buffer size during
a vacuum" or such.

WRT to the TODO specifically: Yes, passing in 0 seems to make sense. I don't
see a reason not to do so? But perhaps there's a better solution:

Perhaps the best solution for autovac vs interactive vacuum issue would be to
move the allocation of the bstrategy to ExecVacuum()?


Random note while looking at the code:
ISTM that adding handling of -1 in GetAccessStrategyWithSize() would make the
code more readable. Instead of

		if (params->ring_size == -1)
		{
			if (VacuumBufferUsageLimit == -1)
				bstrategy = GetAccessStrategy(BAS_VACUUM);
			else
				bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, VacuumBufferUsageLimit);
		}
		else
			bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, params->ring_size);

you could just have something like:
    bstrategy = GetAccessStrategyWithSize(BAS_VACUUM,
        params->ring_size != -1 ? params->ring_size : VacuumBufferUsageLimit);

by falling back to the default values from GetAccessStrategy().

Or even more "extremely", you could entirely remove references to
VacuumBufferUsageLimit and handle that in freelist.c

Greetings,

Andres Freund