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

On Fri, Apr 07, 2023 at 09:12:32AM +1200, David Rowley wrote:
> On Fri, 7 Apr 2023 at 05:20, Melanie Plageman <melanieplageman@gmail.com> wrote:
> > GUC name mentioned in comment is inconsistent with current GUC name.
> >
> > > +/*
> > > + * Upper and lower hard limits for the buffer access strategy ring size
> > > + * specified by the vacuum_buffer_usage_limit GUC and BUFFER_USAGE_LIMIT
> > > + * option to VACUUM and ANALYZE.
> 
> I did adjust this. I wasn't sure it was incorrect as I mentioned "GUC"
> as in, the user facing setting.

Oh, actually maybe you are right then.

> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> index bcc49aec45..220f9ee84c 100644
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -2001,6 +2001,35 @@ include_dir 'conf.d'
>        </listitem>
>       </varlistentry>
>  
> +     <varlistentry id="guc-vacuum-buffer-usage-limit" xreflabel="vacuum_buffer_usage_limit">
> +      <term>
> +       <varname>vacuum_buffer_usage_limit</varname> (<type>integer</type>)
> +       <indexterm>
> +        <primary><varname>vacuum_buffer_usage_limit</varname> configuration parameter</primary>
> +       </indexterm>
> +      </term>
> +      <listitem>
> +       <para>
> +        Specifies the size of <varname>shared_buffers</varname> to be reused
> +        for each backend participating in a given invocation of
> +        <command>VACUUM</command> or <command>ANALYZE</command> or in
> +        autovacuum.  

Rereading this, I think it is not a good sentence (my fault).
Perhaps we should use the same language as the BUFFER_USAGE_LIMIT
options. Something like:

Specifies the
<glossterm linkend="glossary-buffer-access-strategy">Buffer Access Strategy</glossterm>
ring buffer size used by each backend participating in a given
invocation of <command>VACUUM</command> or <command>ANALYZE</command> or
in autovacuum.

Last part is admittedly a bit awkward...

> diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
> index ea1d8960f4..995b4bd54a 100644
> --- a/src/backend/commands/vacuum.c
> +++ b/src/backend/commands/vacuum.c
> @@ -56,6 +56,7 @@
>  #include "utils/acl.h"
>  #include "utils/fmgroids.h"
>  #include "utils/guc.h"
> +#include "utils/guc_hooks.h"
>  #include "utils/memutils.h"
>  #include "utils/pg_rusage.h"
>  #include "utils/snapmgr.h"
> @@ -95,6 +96,26 @@ static VacOptValue get_vacoptval_from_boolean(DefElem *def);
>  static bool vac_tid_reaped(ItemPointer itemptr, void *state);
>  static int	vac_cmp_itemptr(const void *left, const void *right);
>  
> +/*
> + * GUC check function to ensure GUC value specified is within the allowable
> + * range.
> + */
> +bool
> +check_vacuum_buffer_usage_limit(int *newval, void **extra,
> +								GucSource source)
> +{

I'm not so hot on this comment. It seems very...generic. Like it could
be the comment on any GUC check function. I'm also okay with leaving it
as is.

> @@ -341,7 +422,19 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
>  
>  		MemoryContext old_context = MemoryContextSwitchTo(vac_context);
>  
> -		bstrategy = GetAccessStrategy(BAS_VACUUM);
> +		Assert(ring_size >= -1);
> +
> +		/*
> +		 * If BUFFER_USAGE_LIMIT was specified by the VACUUM or ANALYZE
> +		 * command, it overrides the value of VacuumBufferUsageLimit.  Either
> +		 * value may be 0, in which case GetAccessStrategyWithSize() will
> +		 * return NULL, effectively allowing full use of shared buffers.

Maybe "unlimited" is better than "full"?

> +		 */
> +		if (ring_size != -1)
> +			bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, ring_size);
> +		else
> +			bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, VacuumBufferUsageLimit);
> +
>  		MemoryContextSwitchTo(old_context);
>  	}
>  
> diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
> @@ -365,6 +371,9 @@ parallel_vacuum_init(Relation rel, Relation *indrels, int nindexes,
>  		maintenance_work_mem / Min(parallel_workers, nindexes_mwm) :
>  		maintenance_work_mem;
>  
> +	/* Use the same buffer size for all workers */

I would say ring buffer size -- this sounds like it is the size of a
single buffer.

> +	shared->ring_nbuffers = GetAccessStrategyBufferCount(bstrategy);
> +
>  	pg_atomic_init_u32(&(shared->cost_balance), 0);
>  	pg_atomic_init_u32(&(shared->active_nworkers), 0);
>  	pg_atomic_init_u32(&(shared->idx), 0);

> + * Upper and lower hard limits for the buffer access strategy ring size
> + * specified by the VacuumBufferUsageLimit GUC and BUFFER_USAGE_LIMIT option

I agree with your original usage of the actual GUC name, now that I
realize why you were doing it and am rereading it.

> + * to VACUUM and ANALYZE.
> + */
> +#define MIN_BAS_VAC_RING_SIZE_KB 128
> +#define MAX_BAS_VAC_RING_SIZE_KB (16 * 1024 * 1024)


Otherwise, LGTM.