Thanks for the reviews and for trying the patch!
On Wed, Mar 15, 2023 at 4:31 AM Masahiko Sawada <sawada.m...@gmail.com> wrote:
>
> On Sat, Mar 11, 2023 at 11:55 PM Melanie Plageman
> <melanieplage...@gmail.com> wrote:
> >
> > > On Tue, Feb 28, 2023 at 3:16 AM Bharath Rupireddy
> > > <bharath.rupireddyforpostg...@gmail.com> wrote:
> > >
> > > > On Thu, Jan 12, 2023 at 6:06 AM Andres Freund <and...@anarazel.de>
> > > > wrote:
> > > > >
> > > > > On 2023-01-11 17:26:19 -0700, David G. Johnston wrote:
> > > > > > Should we just add "ring_buffers" to the existing "shared_buffers"
> > > > > > and
> > > > > > "temp_buffers" settings?
> > > > >
> > > > > The different types of ring buffers have different sizes, for good
> > > > > reasons. So
> > > > > I don't see that working well. I also think it'd be more often useful
> > > > > to
> > > > > control this on a statement basis - if you have a parallel import
> > > > > tool that
> > > > > starts NCPU COPYs you'd want a smaller buffer than a single threaded
> > > > > COPY. Of
> > > > > course each session can change the ring buffer settings, but still.
> > > >
> > > > How about having GUCs for each ring buffer (bulk_read_ring_buffers,
> > > > bulk_write_ring_buffers, vacuum_ring_buffers - ah, 3 more new GUCs)?
> > > > These options can help especially when statement level controls aren't
> > > > easy to add (COPY, CREATE TABLE AS/CTAS, REFRESH MAT VIEW/RMV)? If
> > > > needed users can also set them at the system level. For instance, one
> > > > can set bulk_write_ring_buffers to other than 16MB or -1 to disable
> > > > the ring buffer to use shared_buffers and run a bunch of bulk write
> > > > queries.
> >
> > In attached v3, I've changed the name of the guc from buffer_usage_limit
> > to vacuum_buffer_usage_limit, since it is only used for vacuum and
> > autovacuum.
> >
> > I haven't added the other suggested strategy gucs, as those could easily
> > be done in a future patchset.
> >
> > I've also changed GetAccessStrategyExt() to GetAccessStrategyWithSize()
> > -- similar to initArrayResultWithSize().
> >
> > And I've added tab completion for BUFFER_USAGE_LIMIT so that it is
> > easier to try out my patch.
> >
> > Most of the TODOs in the code are related to the question of how
> > autovacuum uses the guc vacuum_buffer_usage_limit. autovacuum creates
> > the buffer access strategy ring in do_autovacuum() before looping
> > through and vacuuming tables. It passes this strategy object on to
> > vacuum(). Since we reuse the same strategy object for all tables in a
> > given invocation of do_autovacuum(), only failsafe autovacuum would
> > change buffer access strategies. This is probably okay, but it does mean
> > that the table-level VacuumParams variable, ring_size, means something
> > different for autovacuum than vacuum. Autovacuum workers will always
> > have set it to -1. We won't ever reach code in vacuum() which relies on
> > VacuumParams->ring_size as long as autovacuum workers pass a non-NULL
> > BufferAccessStrategy object to vacuum(), though.
>
> I've not reviewed the patchset in depth yet but I got assertion
> failure and SEGV when using the buffer_usage_limit parameter.
>
> postgres(1:471180)=# vacuum (buffer_usage_limit 10000000000) ;
> 2023-03-15 17:10:02.947 JST [471180] ERROR: buffer_usage_limit for a
> vacuum must be between -1 and 16777216. 10000000000 is invalid. at
> character 9
>
> The message show the max value is 16777216, but when I set it, I got
> an assertion failure:
>
> postgres(1:470992)=# vacuum (buffer_usage_limit 16777216) ;
> TRAP: failed Assert("ring_size < MAX_BAS_RING_SIZE_KB"), File:
> "freelist.c", Line: 606, PID: 470992
>
> Then when I used 1 byte lower value, 16777215, I got a SEGV:
>
> postgres(1:471180)=# vacuum (buffer_usage_limit 16777215) ;
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: 2023-03-15
> 17:10:59.404 JST [471159] LOG: server process (PID 471180) was
> terminated by signal 11: Segmentation fault
>
> Finally, when I used a more lower value, 16777100, I got a memory
> allocation error:
>
> postgres(1:471361)=# vacuum (buffer_usage_limit 16777100) ;
> 2023-03-15 17:12:17.853 JST [471361] ERROR: invalid memory alloc
> request size 18446744073709551572
>
> Probably vacuum_buffer_usage_limit also has the same issue.
Oh dear--it seems I had an integer overflow when calculating the number
of buffers using the specified buffer size in the macro:
#define bufsize_limit_to_nbuffers(bufsize) (bufsize * 1024 / BLCKSZ)
In the attached v4, I've changed that to:
static inline int
bufsize_limit_to_nbuffers(int bufsize_limit_kb)
{
int blcksz_kb = BLCKSZ / 1024;
Assert(blcksz_kb > 0);
return bufsize_limit_kb / blcksz_kb;
}
This should address Justin's suggestions and Ants' concern about the
macro as well.
Also, I was missing the = in the Assert(ring_size <= MAX_BAS_RING_SIZE)
I've fixed that as well, so it should work for you to specify up to 16777216.
> Also, should we support a table option for vacuum_buffer_usage_limit as well?
Hmm. Since this is meant more for balancing resource usage globally, it
doesn't make as much sense as a table option to me. But, I could be
convinced.
On Sat, Mar 11, 2023 at 2:16 PM Justin Pryzby <pry...@telsasoft.com> wrote:
> On Sat, Mar 11, 2023 at 09:55:33AM -0500, Melanie Plageman wrote:
> > Subject: [PATCH v3 2/3] use shared buffers when failsafe active
> >
> > + /*
> > + * Assume the caller who allocated the memory for the
> > + * BufferAccessStrategy object will free it.
> > + */
> > + vacrel->bstrategy = NULL;
>
> This comment could use elaboration:
>
> ** VACUUM normally restricts itself to a small ring buffer; but in
> failsafe mode, in order to process tables as quickly as possible, allow
> the leaving behind large number of dirty buffers.
I've added a comment in attached v4 which is a bit different than Justin's
suggestion but still more verbose than the previous comment.
> > Subject: [PATCH v3 3/3] add vacuum option to specify ring_size and guc
> > + {"vacuum_buffer_usage_limit", PGC_USERSET, RESOURCES_MEM,
> > + gettext_noop("Sets the buffer pool size for
> > operations employing a buffer access strategy."),
>
> The description should mention vacuum, if that's the scope of the GUC's
> behavior.
I've updated this in v4.
> > +#vacuum_buffer_usage_limit = -1 # size of vacuum buffer access strategy
> > ring.
> > + # -1 to use default,
> > + # 0 to disable vacuum buffer access strategy
> > and use shared buffers
> > + # > 0 to specify size
>
> If I'm not wrong, there's still no documentation about "ring buffers" or
> postgres' "strategy". Which seems important to do for this patch, along
> with other documentation.
So, on the topic of "other documentation", I have, at least, added docs
for the vacuum_buffer_usage_limit guc and the BUFFER_USAGE option to
VACUUM and the buffer-usage-limit parameter to vacuumdb.
> This patch should add support in vacuumdb.c. And maybe a comment about
> adding support there, since it's annoying when it the release notes one
> year say "support VACUUM (FOO)" and then one year later say "support
> vacuumdb --foo".
So, v4 adds support for buffer-usage-limit to vacuumdb. There are a few
issues. The main one is that no other vacuumdb option takes a size as a
parameter. I couldn't actually find any other client with a parameter
specified as a size.
My VACUUM option code is using the GUC size parsing code from
parse_int() -- including the unit flag GUC_UNIT_KB. Now that vacuumdb
also needs to parse sizes, I think we'll need to lift the parse_int()
code and the unit_conversion struct and
unit_conversion_memory_unit_conversion_table out of guc.c and put it
somewhere that it can be accessed for more than guc parsing (e.g. option
parsing).
For vacuumdb in this version, I just specified buffer-usage-limit is
only in kB and thus can only be specified as an int.
If we had something like pg_parse_size() in common, would this make
sense? It would be a little bit of work to figure out what to do about
the flags, etc.
Another issue is the server-side guc
#define MAX_BAS_RING_SIZE_KB (16 * 1024 * 1024)
I just redefined it in vacuumdb code. I'm not sure what the preferred
method for dealing with this is.
I know this validation would get done server-side if I just passed the
user-specified option through, but all of the other vacuumdb options
appear to be doing min/max boundary validation on the client side.
On Wed, Mar 15, 2023 at 8:14 PM Justin Pryzby <pry...@telsasoft.com> wrote:
> On Tue, Mar 14, 2023 at 08:56:58PM -0400, Melanie Plageman wrote:
> > On Sat, Mar 11, 2023 at 2:16 PM Justin Pryzby <pry...@telsasoft.com> wrote:
>
> > > > @@ -586,6 +587,45 @@ GetAccessStrategy(BufferAccessStrategyType btype)
> > > > +BufferAccessStrategy
> > > > +GetAccessStrategyWithSize(BufferAccessStrategyType btype, int
> > > > ring_size)
> > >
> > > Maybe it would make sense for GetAccessStrategy() to call
> > > GetAccessStrategyWithSize(). Or maybe not.
> >
> > You mean instead of having anyone call GetAccessStrategyWithSize()?
> > We would need to change the signature of GetAccessStrategy() then -- and
> > there are quite a few callers.
>
> I mean to avoid code duplication, GetAccessStrategy() could "Select ring
> size to use" and then call into GetAccessStrategyWithSize(). Maybe it's
> counter to your intent or otherwise not worth it to save 8 LOC.
Oh, that's a cool idea. I will think on it.
> > > > +#vacuum_buffer_usage_limit = -1 # size of vacuum buffer access
> > > > strategy ring.
> > > > + # -1 to use default,
> > > > + # 0 to disable vacuum buffer access
> > > > strategy and use shared buffers
> > > > + # > 0 to specify size
> > >
> > > If I'm not wrong, there's still no documentation about "ring buffers" or
> > > postgres' "strategy". Which seems important to do for this patch, along
> > > with other documentation.
> >
> > Yes, it is. I have been thinking about where in the docs to add it (the
> > docs about buffer access strategies). Any ideas?
>
> This patch could add something to the vacuum manpage and to the appendix.
> And maybe references from the shared_buffers and pg_buffercache
> manpages.
So, I was thinking it would be good to have some documentation in
general about Buffer Access Strategies (i.e. not just for vacuum). It
would have been nice to have something to reference from the pg_stat_io
docs that describe what buffer access strategies are.
> > > And maybe a comment about adding support there, since it's annoying
> > > when it the release notes one year say "support VACUUM (FOO)" and then
> > > one year later say "support vacuumdb --foo".
> >
> > I'm not sure I totally follow. Do you mean to add a comment to
> > ExecVacuum() saying to add support to vacuumdb when adding a new option
> > to vacuum?
>
> Yeah, like:
> /* Options added here should also be added to vacuumdb.c */
I've added a little something to the comment above the VacuumParams
struct.
> > In the past, did people forget to add support to vacuumdb for new vacuum
> > options or did they forget to document that they did that or did they
> > forgot to include that they did that in the release notes?
>
> The first. Maybe not often, it's not important whether it's in the
> original patch, but it's odd if the vacuumdb option isn't added until
> the following release, which then shows up as a separate "feature".
I've squished in the code for adding the parameter to vacuumdb in a
single commit with the guc and vacuum option, but I will separate it out
after some of the basics get sorted.
- Melanie
From 8e57765196e8af4f53914b813ae88c23cf88ddf8 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Wed, 22 Feb 2023 12:06:41 -0500
Subject: [PATCH v4 1/3] remove global variable vac_strategy
---
src/backend/commands/vacuum.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index c54360a6a0..a6aac30529 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -75,7 +75,6 @@ int vacuum_multixact_failsafe_age;
/* A few variables that don't seem worth passing around as parameters */
static MemoryContext vac_context = NULL;
-static BufferAccessStrategy vac_strategy;
/*
@@ -94,7 +93,7 @@ static void vac_truncate_clog(TransactionId frozenXID,
TransactionId lastSaneFrozenXid,
MultiXactId lastSaneMinMulti);
static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
- bool skip_privs);
+ bool skip_privs, BufferAccessStrategy bstrategy);
static double compute_parallel_delay(void);
static VacOptValue get_vacoptval_from_boolean(DefElem *def);
static bool vac_tid_reaped(ItemPointer itemptr, void *state);
@@ -338,7 +337,7 @@ vacuum(List *relations, VacuumParams *params,
in_outer_xact = IsInTransactionBlock(isTopLevel);
/*
- * Due to static variables vac_context, anl_context and vac_strategy,
+ * Due to static variable vac_context
* vacuum() is not reentrant. This matters when VACUUM FULL or ANALYZE
* calls a hostile index expression that itself calls ANALYZE.
*/
@@ -404,7 +403,6 @@ vacuum(List *relations, VacuumParams *params,
bstrategy = GetAccessStrategy(BAS_VACUUM);
MemoryContextSwitchTo(old_context);
}
- vac_strategy = bstrategy;
/*
* Build list of relation(s) to process, putting any new data in
@@ -509,7 +507,7 @@ vacuum(List *relations, VacuumParams *params,
if (params->options & VACOPT_VACUUM)
{
- if (!vacuum_rel(vrel->oid, vrel->relation, params, false))
+ if (!vacuum_rel(vrel->oid, vrel->relation, params, false, bstrategy))
continue;
}
@@ -527,7 +525,7 @@ vacuum(List *relations, VacuumParams *params,
}
analyze_rel(vrel->oid, vrel->relation, params,
- vrel->va_cols, in_outer_xact, vac_strategy);
+ vrel->va_cols, in_outer_xact, bstrategy);
if (use_own_xacts)
{
@@ -1838,7 +1836,7 @@ vac_truncate_clog(TransactionId frozenXID,
* At entry and exit, we are not inside a transaction.
*/
static bool
-vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs)
+vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs, BufferAccessStrategy bstrategy)
{
LOCKMODE lmode;
Relation rel;
@@ -2084,7 +2082,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs)
cluster_rel(relid, InvalidOid, &cluster_params);
}
else
- table_relation_vacuum(rel, params, vac_strategy);
+ table_relation_vacuum(rel, params, bstrategy);
}
/* Roll back any GUC changes executed by index functions */
@@ -2118,7 +2116,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs)
memcpy(&toast_vacuum_params, params, sizeof(VacuumParams));
toast_vacuum_params.options |= VACOPT_PROCESS_MAIN;
- vacuum_rel(toast_relid, NULL, &toast_vacuum_params, true);
+ vacuum_rel(toast_relid, NULL, &toast_vacuum_params, true, bstrategy);
}
/*
--
2.37.2
From d75a38fe364b5ddfab220cac34a81f069c7415a3 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Wed, 15 Mar 2023 15:47:22 -0400
Subject: [PATCH v4 3/3] add vacuum[db] option to specify ring_size and guc
---
doc/src/sgml/config.sgml | 24 +++++++++
doc/src/sgml/ref/vacuum.sgml | 21 ++++++++
doc/src/sgml/ref/vacuumdb.sgml | 19 +++++++
src/backend/commands/vacuum.c | 51 ++++++++++++++++++-
src/backend/commands/vacuumparallel.c | 6 ++-
src/backend/postmaster/autovacuum.c | 15 +++++-
src/backend/storage/buffer/README | 2 +
src/backend/storage/buffer/freelist.c | 48 +++++++++++++++++
src/backend/utils/init/globals.c | 2 +
src/backend/utils/misc/guc_tables.c | 11 ++++
src/backend/utils/misc/postgresql.conf.sample | 4 ++
src/bin/psql/tab-complete.c | 2 +-
src/bin/scripts/vacuumdb.c | 28 ++++++++++
src/include/commands/vacuum.h | 4 ++
src/include/miscadmin.h | 1 +
src/include/storage/bufmgr.h | 5 ++
16 files changed, 239 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e5c41cc6c6..1a84b5715d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1981,6 +1981,30 @@ 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 ring buffer size to be used for a given invocation of
+ <command>VACUUM</command> or instance of autovacuum. This size is
+ converted to a number of shared buffers which will be reused as part of
+ a <literal>Buffer Access Strategy</literal>. <literal>0</literal> will
+ disable use of a <literal>Buffer Access Strategy</literal>.
+ <literal>-1</literal> will set the size to a default of <literal>256
+ kB</literal>. The maximum ring buffer size is <literal>16 GB</literal>.
+ Though you may set <varname>vacuum_buffer_usage_limit</varname> below
+ <literal>128 kB</literal>, it will be clamped to <literal>128
+ kB</literal> at runtime. The default value is <literal>-1</literal>.
+ This parameter can be set at any time.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-logical-decoding-work-mem" xreflabel="logical_decoding_work_mem">
<term><varname>logical_decoding_work_mem</varname> (<type>integer</type>)
<indexterm>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index b6d30b5764..d0c8adaa61 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -39,6 +39,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
PARALLEL <replaceable class="parameter">integer</replaceable>
SKIP_DATABASE_STATS [ <replaceable class="parameter">boolean</replaceable> ]
ONLY_DATABASE_STATS [ <replaceable class="parameter">boolean</replaceable> ]
+ BUFFER_USAGE_LIMIT [ <replaceable class="parameter">integer</replaceable> ]
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -345,6 +346,26 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>BUFFER_USAGE_LIMIT</literal></term>
+ <listitem>
+ <para>
+ Specifies the ring buffer size for <command>VACUUM</command>. This size
+ is used to calculate a number of shared buffers which will be reused as
+ part of a <literal>Buffer Access Strategy</literal>. <literal>0</literal>
+ disables use of a <literal>Buffer Access Strategy</literal>.
+ <literal>-1</literal> indicates that <command>VACUUM</command> should
+ fall back to the value specified by <xref
+ linkend="guc-vacuum-buffer-usage-limit"/>. The maximum value is
+ <literal>16 GB</literal>. Though you may specify a size smaller than
+ <literal>128</literal>, the value will be clamped to <literal>128
+ kB</literal> at runtime. This size applies to a single invocation of
+ <command>VACUUM</command>. The analyze stage and parallel vacuum workers
+ do not use this size.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">boolean</replaceable></term>
<listitem>
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index 74bac2d4ba..dfc57643a3 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -278,6 +278,25 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--buffer-usage-limit <replaceable class="parameter">buffer_usage_limit</replaceable></option></term>
+ <listitem>
+ <para>
+ The size in kB of the ring buffer used for vacuuming. This size is used
+ to calculate a number of shared buffers which will be reused as part of
+ a <literal>Buffer Access Strategy</literal>. <literal>0</literal>
+ disables use of a <literal>Buffer Access Strategy</literal>.
+ <literal>-1</literal> indicates that the vacuum should fall back to the
+ value specified by <xref linkend="guc-vacuum-buffer-usage-limit"/>. The
+ maximum value is <literal>16777216</literal> (16 GB). Though you may
+ specify a size smaller than <literal>128</literal>, the value will be
+ clamped to <literal>128 kB</literal> at runtime. This size applies to a
+ single invocation of <command>vacuumdb</command>. The analyze stage and
+ parallel vacuum workers do not use this size.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-n <replaceable class="parameter">schema</replaceable></option></term>
<term><option>--schema=<replaceable class="parameter">schema</replaceable></option></term>
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index a6aac30529..8fa09b786a 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -128,6 +128,9 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
/* By default parallel vacuum is enabled */
params.nworkers = 0;
+ /* by default use buffer access strategy with default size */
+ params.ring_size = -1;
+
/* Parse options list */
foreach(lc, vacstmt->options)
{
@@ -211,6 +214,43 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
skip_database_stats = defGetBoolean(opt);
else if (strcmp(opt->defname, "only_database_stats") == 0)
only_database_stats = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "buffer_usage_limit") == 0)
+ {
+ char *vac_buffer_size;
+ int result;
+
+ if (opt->arg == NULL)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("buffer_usage_limit option requires a valid value"),
+ parser_errposition(pstate, opt->location)));
+ }
+
+ vac_buffer_size = defGetString(opt);
+
+ if (!parse_int(vac_buffer_size, &result, GUC_UNIT_KB, NULL))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("buffer_usage_limit for a vacuum must be between -1 and %d. %s is invalid.",
+ MAX_BAS_RING_SIZE_KB, vac_buffer_size),
+ parser_errposition(pstate, opt->location)));
+ }
+
+ /* check for out-of-bounds */
+ if (result < -1 || result > MAX_BAS_RING_SIZE_KB)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("buffer_usage_limit for a vacuum must be between -1 and %d",
+ MAX_BAS_RING_SIZE_KB),
+ parser_errposition(pstate, opt->location)));
+ }
+
+ params.ring_size = result;
+
+ }
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -400,7 +440,16 @@ vacuum(List *relations, VacuumParams *params,
{
MemoryContext old_context = MemoryContextSwitchTo(vac_context);
- bstrategy = GetAccessStrategy(BAS_VACUUM);
+ if (params->ring_size == -1)
+ {
+ if (vacuum_buffer_usage_limit == -1)
+ bstrategy = GetAccessStrategy(BAS_VACUUM);
+ else
+ bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, vacuum_buffer_usage_limit);
+ }
+ else
+ bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, params->ring_size);
+
MemoryContextSwitchTo(old_context);
}
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index bcd40c80a1..16742f6612 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -1012,7 +1012,11 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
pvs.indname = NULL;
pvs.status = PARALLEL_INDVAC_STATUS_INITIAL;
- /* Each parallel VACUUM worker gets its own access strategy */
+ /*
+ * Each parallel VACUUM worker gets its own access strategy
+ * For now, use the default buffer access strategy ring size.
+ * TODO: should this work differently even though they are only for indexes
+ */
pvs.bstrategy = GetAccessStrategy(BAS_VACUUM);
/* Setup error traceback support for ereport() */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index c0e2e00a7e..7dbd3b8935 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2291,8 +2291,14 @@ do_autovacuum(void)
* Create a buffer access strategy object for VACUUM to use. We want to
* use the same one across all the vacuum operations we perform, since the
* point is for VACUUM not to blow out the shared cache.
+ * If we later enter failsafe mode, we will cease use of the
+ * BufferAccessStrategy. Either way, we clean up the BufferAccessStrategy
+ * object at the end of this function.
*/
- bstrategy = GetAccessStrategy(BAS_VACUUM);
+ if (vacuum_buffer_usage_limit == -1)
+ bstrategy = GetAccessStrategy(BAS_VACUUM);
+ else
+ bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, vacuum_buffer_usage_limit);
/*
* create a memory context to act as fake PortalContext, so that the
@@ -2881,6 +2887,13 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
tab->at_params.multixact_freeze_table_age = multixact_freeze_table_age;
tab->at_params.is_wraparound = wraparound;
tab->at_params.log_min_duration = log_min_duration;
+
+ // TODO: should this be 0 so that we are sure that vacuum() never
+ // allocates a new bstrategy for us, even if we pass in NULL for that
+ // parameter? maybe could change how failsafe NULLs out bstrategy if
+ // so?
+ tab->at_params.ring_size = vacuum_buffer_usage_limit;
+
tab->at_vacuum_cost_limit = vac_cost_limit;
tab->at_vacuum_cost_delay = vac_cost_delay;
tab->at_relname = NULL;
diff --git a/src/backend/storage/buffer/README b/src/backend/storage/buffer/README
index a775276ff2..bf166bbdf1 100644
--- a/src/backend/storage/buffer/README
+++ b/src/backend/storage/buffer/README
@@ -245,6 +245,8 @@ for WAL flushes. While it's okay for a background vacuum to be slowed by
doing its own WAL flushing, we'd prefer that COPY not be subject to that,
so we let it use up a bit more of the buffer arena.
+TODO: update with info about new option to control the size
+
Background Writer's Processing
------------------------------
diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
index c690d5f15f..f12c9224d7 100644
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -586,6 +586,54 @@ GetAccessStrategy(BufferAccessStrategyType btype)
return strategy;
}
+static inline int
+bufsize_limit_to_nbuffers(int bufsize_limit_kb)
+{
+ int blcksz_kb = BLCKSZ / 1024;
+
+ Assert(blcksz_kb > 0);
+
+ return bufsize_limit_kb / blcksz_kb;
+}
+
+BufferAccessStrategy
+GetAccessStrategyWithSize(BufferAccessStrategyType btype, int ring_size)
+{
+ BufferAccessStrategy strategy;
+ int nbuffers;
+
+ /* Default nbuffers should have resulted in calling GetAccessStrategy() */
+ // TODO: this being called ring_size and also nbuffers being called
+ // ring_size in GetAccessStrategy is confusing. Rename the member of
+ // BufferAccessStrategy
+ Assert(ring_size != -1);
+
+ if (ring_size == 0)
+ return NULL;
+
+ Assert(ring_size <= MAX_BAS_RING_SIZE_KB);
+
+ // TODO: warn about clamping?
+ if (ring_size < MIN_BAS_RING_SIZE_KB)
+ nbuffers = bufsize_limit_to_nbuffers(MIN_BAS_RING_SIZE_KB);
+ else
+ nbuffers = bufsize_limit_to_nbuffers(ring_size);
+
+ // TODO: make this smaller?
+ nbuffers = Min(NBuffers / 8, nbuffers);
+
+ /* Allocate the object and initialize all elements to zeroes */
+ strategy = (BufferAccessStrategy)
+ palloc0(offsetof(BufferAccessStrategyData, buffers) +
+ nbuffers * sizeof(Buffer));
+
+ /* Set fields that don't start out zero */
+ strategy->btype = btype;
+ strategy->ring_size = nbuffers;
+
+ return strategy;
+}
+
/*
* FreeAccessStrategy -- release a BufferAccessStrategy object
*
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 1b1d814254..6eca3371bd 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -139,6 +139,8 @@ int max_worker_processes = 8;
int max_parallel_workers = 8;
int MaxBackends = 0;
+int vacuum_buffer_usage_limit = -1;
+
int VacuumCostPageHit = 1; /* GUC parameters for vacuum */
int VacuumCostPageMiss = 2;
int VacuumCostPageDirty = 20;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1c0583fe26..883ee29d14 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2206,6 +2206,17 @@ struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"vacuum_buffer_usage_limit", PGC_USERSET, RESOURCES_MEM,
+ gettext_noop("Sets the buffer pool size for VACUUM and autovacuum."),
+ NULL,
+ GUC_UNIT_KB
+ },
+ &vacuum_buffer_usage_limit,
+ -1, -1, MAX_BAS_RING_SIZE_KB,
+ NULL, NULL, NULL
+ },
+
{
{"shared_memory_size", PGC_INTERNAL, PRESET_OPTIONS,
gettext_noop("Shows the size of the server's main shared memory area (rounded up to the nearest MB)."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index d06074b86f..36273aa47d 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -156,6 +156,10 @@
# mmap
# (change requires restart)
#min_dynamic_shared_memory = 0MB # (change requires restart)
+#vacuum_buffer_usage_limit = -1 # size of vacuum buffer access strategy ring.
+ # -1 to use default,
+ # 0 to disable vacuum buffer access strategy and use shared buffers
+ # > 0 to specify size
# - Disk -
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 42e87b9e49..6fd80dd3c3 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -4620,7 +4620,7 @@ psql_completion(const char *text, int start, int end)
"DISABLE_PAGE_SKIPPING", "SKIP_LOCKED",
"INDEX_CLEANUP", "PROCESS_MAIN", "PROCESS_TOAST",
"TRUNCATE", "PARALLEL", "SKIP_DATABASE_STATS",
- "ONLY_DATABASE_STATS");
+ "ONLY_DATABASE_STATS", "BUFFER_USAGE_LIMIT");
else if (TailMatches("FULL|FREEZE|ANALYZE|VERBOSE|DISABLE_PAGE_SKIPPING|SKIP_LOCKED|PROCESS_MAIN|PROCESS_TOAST|TRUNCATE|SKIP_DATABASE_STATS|ONLY_DATABASE_STATS"))
COMPLETE_WITH("ON", "OFF");
else if (TailMatches("INDEX_CLEANUP"))
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 39be265b5b..74e50ac286 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -25,6 +25,7 @@
#include "fe_utils/simple_list.h"
#include "fe_utils/string_utils.h"
+#define MAX_BAS_RING_SIZE_KB (16 * 1024 * 1024)
/* vacuum options controlled by user flags */
typedef struct vacuumingOptions
@@ -46,6 +47,7 @@ typedef struct vacuumingOptions
bool process_main;
bool process_toast;
bool skip_database_stats;
+ int buffer_usage_limit;
} vacuumingOptions;
/* object filter options */
@@ -123,6 +125,7 @@ main(int argc, char *argv[])
{"no-truncate", no_argument, NULL, 10},
{"no-process-toast", no_argument, NULL, 11},
{"no-process-main", no_argument, NULL, 12},
+ {"buffer-usage-limit", required_argument, NULL, 13},
{NULL, 0, NULL, 0}
};
@@ -147,6 +150,7 @@ main(int argc, char *argv[])
/* initialize options */
memset(&vacopts, 0, sizeof(vacopts));
vacopts.parallel_workers = -1;
+ vacopts.buffer_usage_limit = -1;
vacopts.no_index_cleanup = false;
vacopts.force_index_cleanup = false;
vacopts.do_truncate = true;
@@ -266,6 +270,12 @@ main(int argc, char *argv[])
case 12:
vacopts.process_main = false;
break;
+ case 13:
+ if (!option_parse_int(optarg, "--buffer-usage-limit",
+ -1, MAX_BAS_RING_SIZE_KB,
+ &vacopts.buffer_usage_limit))
+ exit(1);
+ break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
@@ -324,6 +334,9 @@ main(int argc, char *argv[])
if (!vacopts.process_toast)
pg_fatal("cannot use the \"%s\" option when performing only analyze",
"no-process-toast");
+ if (vacopts.buffer_usage_limit != -1)
+ pg_fatal("cannot use the \"%s\" option when performing only analyze",
+ "buffer-usage-limit");
/* allow 'and_analyze' with 'analyze_only' */
}
@@ -550,6 +563,13 @@ vacuum_one_database(ConnParams *cparams,
pg_fatal("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
"--parallel", "13");
+ // TODO: this is a problem: if the user specifies this option with -1 in a
+ // version before 16, it will not produce an error message. it also won't
+ // do anything, but that still doesn't seem right.
+ if (vacopts->buffer_usage_limit > -1 && PQserverVersion(conn) < 160000)
+ pg_fatal("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
+ "--buffer-usage-limit", "16");
+
/* skip_database_stats is used automatically if server supports it */
vacopts->skip_database_stats = (PQserverVersion(conn) >= 160000);
@@ -1048,6 +1068,13 @@ prepare_vacuum_command(PQExpBuffer sql, int serverVersion,
vacopts->parallel_workers);
sep = comma;
}
+ if (vacopts->buffer_usage_limit > -1)
+ {
+ Assert(serverVersion >= 160000);
+ appendPQExpBuffer(sql, "%sBUFFER_USAGE_LIMIT %d", sep,
+ vacopts->buffer_usage_limit);
+ sep = comma;
+ }
if (sep != paren)
appendPQExpBufferChar(sql, ')');
}
@@ -1111,6 +1138,7 @@ help(const char *progname)
printf(_(" --force-index-cleanup always remove index entries that point to dead tuples\n"));
printf(_(" -j, --jobs=NUM use this many concurrent connections to vacuum\n"));
printf(_(" --min-mxid-age=MXID_AGE minimum multixact ID age of tables to vacuum\n"));
+ printf(_(" --buffer-usage-limit=BUFSIZE_KB size of ring buffer in kB used for vacuum\n"));
printf(_(" --min-xid-age=XID_AGE minimum transaction ID age of tables to vacuum\n"));
printf(_(" --no-index-cleanup don't remove index entries that point to dead tuples\n"));
printf(_(" --no-process-main skip the main relation\n"));
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bdfd96cfec..63755480cf 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -213,6 +213,9 @@ typedef enum VacOptValue
*
* Note that at least one of VACOPT_VACUUM and VACOPT_ANALYZE must be set
* in options.
+ *
+ * When adding a new VacuumParam member, consider adding it to vacuumdb as
+ * well.
*/
typedef struct VacuumParams
{
@@ -236,6 +239,7 @@ typedef struct VacuumParams
* disabled.
*/
int nworkers;
+ int ring_size;
} VacuumParams;
/*
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 06a86f9ac1..b572dfcc6c 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -262,6 +262,7 @@ extern PGDLLIMPORT int work_mem;
extern PGDLLIMPORT double hash_mem_multiplier;
extern PGDLLIMPORT int maintenance_work_mem;
extern PGDLLIMPORT int max_parallel_maintenance_workers;
+extern PGDLLIMPORT int vacuum_buffer_usage_limit;
extern PGDLLIMPORT int VacuumCostPageHit;
extern PGDLLIMPORT int VacuumCostPageMiss;
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index b8a18b8081..338de38568 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -101,6 +101,9 @@ extern PGDLLIMPORT int32 *LocalRefCount;
/* upper limit for effective_io_concurrency */
#define MAX_IO_CONCURRENCY 1000
+#define MAX_BAS_RING_SIZE_KB (16 * 1024 * 1024)
+#define MIN_BAS_RING_SIZE_KB 128
+
/* special block number for ReadBuffer() */
#define P_NEW InvalidBlockNumber /* grow the file to get a new page */
@@ -196,6 +199,8 @@ extern void AtProcExit_LocalBuffers(void);
/* in freelist.c */
extern BufferAccessStrategy GetAccessStrategy(BufferAccessStrategyType btype);
+
+extern BufferAccessStrategy GetAccessStrategyWithSize(BufferAccessStrategyType btype, int nbuffers);
extern void FreeAccessStrategy(BufferAccessStrategy strategy);
--
2.37.2
From 55a4cb0356951582acfe920112ed887f80670ce4 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Wed, 22 Feb 2023 12:26:01 -0500
Subject: [PATCH v4 2/3] use shared buffers when failsafe active
---
src/backend/access/heap/vacuumlazy.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 8f14cf85f3..3de7976cf6 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -2622,6 +2622,13 @@ lazy_check_wraparound_failsafe(LVRelState *vacrel)
if (unlikely(vacuum_xid_failsafe_check(&vacrel->cutoffs)))
{
vacrel->failsafe_active = true;
+ /*
+ * Abandon use of a buffer access strategy when entering failsafe mode,
+ * as completing the ongoing VACUUM is our top priority.
+ * Assume the caller who allocated the memory for the
+ * BufferAccessStrategy object will free it.
+ */
+ vacrel->bstrategy = NULL;
/* Disable index vacuuming, index cleanup, and heap rel truncation */
vacrel->do_index_vacuuming = false;
--
2.37.2
