From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Melanie Plageman <melanieplageman(at)gmail(dot)com> |
Cc: | David Rowley <dgrowleyml(at)gmail(dot)com>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Peter Geoghegan <pg(at)bowt(dot)ie>, pgsql-hackers(at)postgresql(dot)org, Robert Haas <robertmhaas(at)gmail(dot)com> |
Subject: | Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode |
Date: | 2023-04-05 17:05:53 |
Message-ID: | 20230405170553.webeqbzhmnawgnvg@awork3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Greg Stark | 2023-04-05 17:19:42 | Re: Schema variables - new implementation for Postgres 15 |
Previous Message | Tom Lane | 2023-04-05 17:05:34 | Re: Why enable_hashjoin Completely disables HashJoin |