On Fri, Apr 07, 2023 at 09:12:32AM +1200, David Rowley wrote:
> On Fri, 7 Apr 2023 at 05:20, Melanie Plageman <melanieplage...@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.
