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

From: Melanie Plageman <[email-masked]>
Date: Thu, 6 Apr 2023 17:44:31 -0400

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

---

From: David Rowley <[email-masked]>
Date: Fri, 7 Apr 2023 12:52:03 +1200

> Otherwise, LGTM.

Thanks for looking.  I've also taken Justin's comments about the
README into account and fixed that part.

I've pushed the patch after a little more adjusting.  I added some
text to the docs that mention larger VACUUM_BUFFER_LIMITs can speed up
vacuum and also a reason why they might not want to go nuts with it.

I've also just now pushed the vacuumdb patch too. I ended up adjusting
some of the ERROR messages in the main patch after the following not
so nice user experience:

$ vacuumdb --buffer-usage-limit=1TB --analyze postgres
vacuumdb: vacuuming database "postgres"
SQL: VACUUM (SKIP_DATABASE_STATS, ANALYZE, BUFFER_USAGE_LIMIT '1TB')
vacuumdb: error: processing of database "postgres" failed: ERROR:
buffer_usage_limit option must be 0 or between 128 KB and 16777216 KB

$ vacuumdb --buffer-usage-limit=128KB --analyze postgres
vacuumdb: vacuuming database "postgres"
SQL: VACUUM (SKIP_DATABASE_STATS, ANALYZE, BUFFER_USAGE_LIMIT '128KB')
vacuumdb: error: processing of database "postgres" failed: ERROR:
value: "128KB": is invalid for buffer_usage_limit
HINT:  Valid units for this parameter are "B", "kB", "MB", "GB", and "TB".

David

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 14 Apr 2023 09:20:41 +0200

> vacuum and also a reason why they might not want to go nuts with it.

I came across these new options and had a little bit of trouble figuring 
them out from the documentation.  Maybe this could be polished a bit.

vacuumdb --help says

     --buffer-usage-limit=BUFSIZE

I can guess what a "SIZE" might be, but is "BUFSIZE" different from a 
"SIZE"?  Maybe simplify here.

On the vacuumdb man page, the placeholder is

     <replaceable class="parameter">buffer_usage_limit</replaceable>

which is yet another way of phrasing it.  Maybe also use "size" here?

The VACUUM man page says

     BUFFER_USAGE_LIMIT [ <replaceable ...>string</replaceable> ]

which had me really confused.  The detailed description later doesn't 
give any further explanation of possible values, except that 
<literal>0</literal> is apparently a possible value, which in my mind is 
not a string.  Then there is a link to guc-vacuum-buffer-usage-limit, 
which lifts the mystery that this is really just an integer setting with 
possible memory-size units, but it was really hard to figure that out 
from the start!

Moreover, on the VACUUM man page, right below BUFFER_USAGE_LIMIT, it 
explains the different kinds of accepted values, and "string" wasn't 
added there.  Maybe also change this to "size" here and add an 
explanation there what kinds of sizes are possible.

Finally, the locations of the new options in the various documentation 
places seems a bit random.  The vacuumdb --help output and the man page 
appear to be mostly alphabetical, so --buffer-usage-limit should be 
after -a/--all.  (Also note that right now the option isn't even in the 
same place in the --help output versus the man page.)

The order of the options on the VACUUM man page doesn't make any sense 
anymore.  This isn't really the fault of this patch, but maybe it's time 
to do a fresh reordering there.

---

From: David Rowley <[email-masked]>
Date: Sat, 15 Apr 2023 12:59:52 +1200

> same place in the --help output versus the man page.)

These are all valid points. I've attached a patch aiming to address
each of them.

> to do a fresh reordering there.

Agreed, that likely wasn't a big problem say about 5 years ago when we
had far fewer options, but the number has grown quite a bit since
then.

Right after I fix the points you've mentioned seems a good time to address that.

David

---

From: David Rowley <[email-masked]>
Date: Sun, 16 Apr 2023 12:09:44 +1200

> These are all valid points. I've attached a patch aiming to address

I tweaked this a little further and pushed it.

David

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 26 Apr 2023 17:47:43 +0900

Hi,

> I tweaked this a little further and pushed it.

I realized that the value of vacuum_buffer_usage_limit parameter in
postgresql.conf.sample doesn't have the unit:

#vacuum_buffer_usage_limit = 256 # size of vacuum and analyze buffer
access strategy ring.
                                 # 0 to disable vacuum buffer access strategy
                                 # range 128kB to 16GB

It works but I think we might want to add the unit kB for
understandability and consistency with other GUC_UNIT_KB parameters.
I've attached a small patch that adds the unit and aligns the indent
of the comments to the perimeter parameters.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

---

From: David Rowley <[email-masked]>
Date: Wed, 26 Apr 2023 23:26:43 +1200

> of the comments to the perimeter parameters.

I'm not currently able to check, but if work_mem has a unit in the sample
conf then I agree that vacuum_buffer_usage_limit should too.

I'm fine for you to go ahead and adjust this, otherwise it'll be Monday
before I can.

David