Обсуждение: pgsql: Move various prechecks from vacuum() into ExecVacuum()
Move various prechecks from vacuum() into ExecVacuum() vacuum() is used for both the VACUUM command and for autovacuum. There were many prechecks being done inside vacuum() that were just not relevant to autovacuum. Let's move the bulk of these into ExecVacuum() so that they're only executed when running the VACUUM command. This removes a small amount of overhead when autovacuum vacuums a table. While we are at it, allocate VACUUM's BufferAccessStrategy in ExecVacuum() and pass it into vacuum() instead of expecting vacuum() to make it if it's not already made by the calling function. To make this work, we need to create the vacuum memory context slightly earlier, so we now need to pass that down to vacuum() so that it's available for use in other memory allocations. Author: Melanie Plageman Reviewed-by: David Rowley Discussion: https://postgr.es/m/20230405211534.4skgskbilnxqrmxg@awork3.anarazel.de Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/b9b125b9c14381c4d04a446e335bb2da5f602596 Modified Files -------------- src/backend/commands/vacuum.c | 154 ++++++++++++++++++------------------ src/backend/postmaster/autovacuum.c | 12 ++- src/include/commands/vacuum.h | 3 +- 3 files changed, 91 insertions(+), 78 deletions(-)
Hi David, On Thu, Apr 06, 2023 at 03:45:19AM +0000, David Rowley wrote: > Move various prechecks from vacuum() into ExecVacuum() > > vacuum() is used for both the VACUUM command and for autovacuum. There > were many prechecks being done inside vacuum() that were just not relevant > to autovacuum. Let's move the bulk of these into ExecVacuum() so that > they're only executed when running the VACUUM command. This removes a > small amount of overhead when autovacuum vacuums a table. > > While we are at it, allocate VACUUM's BufferAccessStrategy in ExecVacuum() > and pass it into vacuum() instead of expecting vacuum() to make it if it's > not already made by the calling function. To make this work, we need to > create the vacuum memory context slightly earlier, so we now need to pass > that down to vacuum() so that it's available for use in other memory > allocations. I have just seen this commit, and I am pretty sure that the checks have been placed in vacuum() to guard against incorrect option manipulations in the context of an autovacuum building the relations, so you are making this code weaker with the cross-checks it had, IMO. This is particularly relevant in some areas with toast relations, for example, because autovacuum handles the toast and their parents separately, contrary to ExecVacuum() that would group them together by default. My 2c. -- Michael
Вложения
Hi, On 2023-04-06 13:04:17 +0900, Michael Paquier wrote: > On Thu, Apr 06, 2023 at 03:45:19AM +0000, David Rowley wrote: > > Move various prechecks from vacuum() into ExecVacuum() > > > > vacuum() is used for both the VACUUM command and for autovacuum. There > > were many prechecks being done inside vacuum() that were just not relevant > > to autovacuum. Let's move the bulk of these into ExecVacuum() so that > > they're only executed when running the VACUUM command. This removes a > > small amount of overhead when autovacuum vacuums a table. > > > > While we are at it, allocate VACUUM's BufferAccessStrategy in ExecVacuum() > > and pass it into vacuum() instead of expecting vacuum() to make it if it's > > not already made by the calling function. To make this work, we need to > > create the vacuum memory context slightly earlier, so we now need to pass > > that down to vacuum() so that it's available for use in other memory > > allocations. > > I have just seen this commit, and I am pretty sure that the checks > have been placed in vacuum() to guard against incorrect option > manipulations in the context of an autovacuum building the relations, > so you are making this code weaker with the cross-checks it had, IMO. > This is particularly relevant in some areas with toast relations, for > example, because autovacuum handles the toast and their parents > separately, contrary to ExecVacuum() that would group them together by > default. They're prety useless for that though. You don't even see those errors anywhere. For the purpose of validating that autovac does something sensible you just need an Assert(). And it can be in do_autovacuum() or in vacuum(). You don't need translated ereport()s. Greetings, Andres Freund
On Thu, 6 Apr 2023 at 16:04, Michael Paquier <michael@paquier.xyz> wrote: > I have just seen this commit, and I am pretty sure that the checks > have been placed in vacuum() to guard against incorrect option > manipulations in the context of an autovacuum building the relations, > so you are making this code weaker with the cross-checks it had, IMO. > This is particularly relevant in some areas with toast relations, for > example, because autovacuum handles the toast and their parents > separately, contrary to ExecVacuum() that would group them together by > default. hmm, I'm not sure I agree that would warrant keeping ereport()s in vacuum(). Autovacuum would have to set either VACOPT_FULL to possibly trigger the first two moved ereport()s and the final 2 would need VACOPT_ONLY_DATABASE_STATS. None of those are ever set by auto-vacuum, so it seems unlikely that some logic somewhere like relation_needs_vacanalyze(). Asserts() might be a good compromise. David