Re: Decoupling antiwraparound autovacuum from special rules around auto cancellation - Mailing list pgsql-hackers

Hi,

On 2023-01-18 17:54:27 -0800, Peter Geoghegan wrote:
> From 0afaf310255a068d3c1ca9d2ce6f00118cbff890 Mon Sep 17 00:00:00 2001
> From: Peter Geoghegan <pg@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 <pg@bowt.ie>
> Reviewed-By: Andres Freund <andres@anarazel.de>
> Reviewed-By: Jeff Davis <pgsql@j-davis.com>
> Discussion: https://postgr.es/m/CAH2-Wz=S-R_2rO49Hm94Nuvhu9_twRGbTm6uwDRmRu-Sqn_t3w@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