Hi,
On 2023-01-18 17:54:27 -0800, Peter Geoghegan wrote:
> From 0afaf310255a068d3c1ca9d2ce6f00118cbff890 Mon Sep 17 00:00:00 2001
> From: Peter Geoghegan <p...@bowt.ie>
> Date: Fri, 25 Nov 2022 11:23:20 -0800
> Subject: [PATCH v5 1/2] Add autovacuum trigger instrumentation.
>
> Add new instrumentation that lists a triggering condition in the server
> log whenever an autovacuum is logged. This reports "table age" as the
> triggering criteria when antiwraparound autovacuum runs (the XID age
> trigger case and the MXID age trigger case are represented separately).
> Other cases are reported as autovacuum trigger when the tuple insert
> thresholds or the dead tuple thresholds were crossed.
>
> Author: Peter Geoghegan <p...@bowt.ie>
> Reviewed-By: Andres Freund <and...@anarazel.de>
> Reviewed-By: Jeff Davis <pg...@j-davis.com>
> Discussion:
> https://postgr.es/m/CAH2-Wz=s-r_2ro49hm94nuvhu9_twrgbtm6uwdrmru-sqn_...@mail.gmail.com
> ---
> src/include/commands/vacuum.h | 19 +++-
> src/backend/access/heap/vacuumlazy.c | 5 ++
> src/backend/commands/vacuum.c | 31 ++++++-
> src/backend/postmaster/autovacuum.c | 124 ++++++++++++++++++---------
> 4 files changed, 137 insertions(+), 42 deletions(-)
>
> diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
> index 689dbb770..13f70a1f6 100644
> --- a/src/include/commands/vacuum.h
> +++ b/src/include/commands/vacuum.h
> @@ -191,6 +191,21 @@ typedef struct VacAttrStats
> #define VACOPT_SKIP_DATABASE_STATS 0x100 /* skip
> vac_update_datfrozenxid() */
> #define VACOPT_ONLY_DATABASE_STATS 0x200 /* only
> vac_update_datfrozenxid() */
>
> +/*
> + * Values used by autovacuum.c to tell vacuumlazy.c about the specific
> + * threshold type that triggered an autovacuum worker.
> + *
> + * AUTOVACUUM_NONE is used when VACUUM isn't running in an autovacuum worker.
> + */
> +typedef enum AutoVacType
> +{
> + AUTOVACUUM_NONE = 0,
> + AUTOVACUUM_TABLE_XID_AGE,
> + AUTOVACUUM_TABLE_MXID_AGE,
> + AUTOVACUUM_DEAD_TUPLES,
> + AUTOVACUUM_INSERTED_TUPLES,
> +} AutoVacType;
Why is there TABLE_ in AUTOVACUUM_TABLE_XID_AGE but not
AUTOVACUUM_DEAD_TUPLES? Both are on tables.
What do you think about naming this VacuumTriggerType and adding an
VAC_TRIG_MANUAL or such?
> /*
> * Values used by index_cleanup and truncate params.
> *
> @@ -222,7 +237,8 @@ typedef struct VacuumParams
>
> * use default */
> int multixact_freeze_table_age; /* multixact age at
> which to scan
>
> * whole table */
> - bool is_wraparound; /* force a for-wraparound vacuum */
> + bool is_wraparound; /* antiwraparound autovacuum? */
> + AutoVacType trigger; /* autovacuum trigger condition, if any
> */
The comment change for is_wraparound seems a bit pointless, but whatever.
> @@ -2978,7 +2995,10 @@ relation_needs_vacanalyze(Oid relid,
> bool *doanalyze,
> bool *wraparound)
> {
The changes here are still bigger than I'd like, but ...
> - bool force_vacuum;
> + TransactionId relfrozenxid = classForm->relfrozenxid;
> + MultiXactId relminmxid = classForm->relminmxid;
> + AutoVacType trigger = AUTOVACUUM_NONE;
> + bool tableagevac;
Here + below we end up with three booleans that just represent the choices in
our fancy new enum. That seems awkward to me.
> @@ -3169,14 +3212,15 @@ autovacuum_do_vac_analyze(autovac_table *tab,
> BufferAccessStrategy bstrategy)
> static void
> autovac_report_activity(autovac_table *tab)
> {
> -#define MAX_AUTOVAC_ACTIV_LEN (NAMEDATALEN * 2 + 56)
> +#define MAX_AUTOVAC_ACTIV_LEN (NAMEDATALEN * 2 + 100)
> char activity[MAX_AUTOVAC_ACTIV_LEN];
> int len;
>
> /* Report the command and possible options */
> if (tab->at_params.options & VACOPT_VACUUM)
> snprintf(activity, MAX_AUTOVAC_ACTIV_LEN,
> - "autovacuum: VACUUM%s",
> + "autovacuum for %s: VACUUM%s",
> +
> vac_autovacuum_trigger_msg(tab->at_params.trigger),
> tab->at_params.options & VACOPT_ANALYZE ? "
> ANALYZE" : "");
> else
> snprintf(activity, MAX_AUTOVAC_ACTIV_LEN,
Somehow the added "for ..." sounds a bit awkward. "autovacuum for table XID
age". Maybe "autovacuum due to ..."?
Greetings,
Andres Freund
