Thank you to all reviewers!
This email is in answer to the three reviews
done since my last version. Attached is v2 and inline below is replies
to all the review comments.
The main difference between this version and the previous version is
that I've added a guc, buffer_usage_limit and the VACUUM option
BUFFER_USAGE_LIMIT is now to be specified in size (like kB, MB, etc).
I currently only use the guc value for VACUUM, but it is meant to be
used for all buffer access strategies and is configurable at the session
level.
I would prefer that we had the option of resizing the buffer access
strategy object per table being autovacuumed. Since autovacuum reloads
the config file between tables, this would be quite possible.
I started implementing this, but stopped because the code is not really
in a good state for that.
In fact, I'm not very happy with my implementation at all because I
think given the current structure of vacuum() and vacuum_rel(), it will
potentially make the code more confusing.
I don't like how autovacuum and vacuum use vacuum_rel() and vacuum()
differently (autovacuum always calls vacuum() with a list containing a
single relation). And vacuum() takes buffer access strategy as a
parameter, supposedly so that autovacuum can change the buffer access
strategy object per call, but it doesn't do that. And then vacuum() and
vacuum_rel() go and access VacuumParams at various places with no rhyme
or reason -- seemingly just based on the random availability of other
objects whose state they would like to check on. So, IMO, in adding a
"buffers" parameter to VacuumParams, I am asking for confusion in
autovacuum code with table-level VacuumParams containing an value for
buffers that isn't used.
On Mon, Feb 27, 2023 at 4:21 PM Andres Freund <and...@anarazel.de> wrote:
> On 2023-02-22 16:32:53 -0500, Melanie Plageman wrote:
> > The first patch in the set is to free the BufferAccessStrategy object
> > that is made in do_autovacuum() -- I don't see when the memory context
> > it is allocated in is destroyed, so it seems like it might be a leak?
>
> The backend shuts down just after, so that's not a real issue. Not that it'd
> hurt to fix it.
I've dropped that patch from the set.
> > > I can see that making sense, particularly if we were to later extend this
> > > to
> > > other users of ringbuffers. E.g. COPYs us of the ringbuffer makes loading
> > > of
> > > data > 16MB but also << s_b vastly slower, but it can still be very
> > > important
> > > to use if there's lots of parallel processes loading data.
> > >
> > > Maybe BUFFER_USAGE_LIMIT, with a value from -1 to N, with -1 indicating
> > > the
> > > default value, 0 preventing use of a buffer access strategy, and 1..N
> > > indicating the size in blocks?
>
> > I have found the implementation you suggested very hard to use.
> > The attached fourth patch in the set implements it the way you suggest.
> > I had to figure out what number to set the BUFER_USAGE_LIMIT to -- and,
> > since I don't specify shared buffers in units of nbuffer, it's pretty
> > annoying to have to figure out a valid number.
>
> I think we should be able to parse it in a similar way to how we parse
> shared_buffers. You could even implement this as a GUC that is then set by
> VACUUM (similar to how VACUUM FREEZE is implemented).
in the attached v2, I've used parse_int() to do this.
> > I think that it would be better to have it be either a percentage of
> > shared buffers or a size in units of bytes/kb/mb like that of shared
> > buffers.
>
> I don't think a percentage of shared buffers works particularly well - you
> very quickly run into the ringbuffer becoming impractically big.
It is now a size.
> > > Would we want to set an upper limit lower than implied by the memory limit
> > > for
> > > the BufferAccessStrategy allocation?
> > >
> > >
> > So, I was wondering what you thought about NBuffers / 8 (the current
> > limit). Does it make sense?
>
> That seems *way* too big. Imagine how large allocations we'd end up with a
> shared_buffers size of a few TB.
>
> I'd probably make it a hard error at 1GB and a silent cap at NBuffers / 2 or
> such.
Well, as I mentioned NBuffers / 8 is the current GetAccessStrategy()
cap.
In the attached patchset, I have introduced a hard cap of 16GB which is
enforced for the VACUUM option and for the buffer_usage_limit guc. I
kept the "silent cap" at NBuffers / 8 but am open to changing it to
NBuffers / 2 if we think it is okay for its silent cap to be different
than GetAccessStrategy()'s cap.
> > @@ -547,7 +547,7 @@ bt_check_every_level(Relation rel, Relation heaprel,
> > bool heapkeyspace,
> > state->targetcontext = AllocSetContextCreate(CurrentMemoryContext,
> >
> > "amcheck context",
> >
> > ALLOCSET_DEFAULT_SIZES);
> > - state->checkstrategy = GetAccessStrategy(BAS_BULKREAD);
> > + state->checkstrategy = GetAccessStrategy(BAS_BULKREAD, -1);
> >
> > /* Get true root block from meta-page */
> > metapage = palloc_btree_page(state, BTREE_METAPAGE);
>
> Changing this everywhere seems pretty annoying, particularly because I suspect
> a bunch of extensions also use GetAccessStrategy(). How about a
> GetAccessStrategyExt(), GetAccessStrategyCustomSize() or such?
Yes, I don't know what I was thinking. Changed it to
GetAccessStrategyExt() -- though now I am thinking I don't like Ext and
want to change it.
> > BufferAccessStrategy
> > -GetAccessStrategy(BufferAccessStrategyType btype)
> > +GetAccessStrategy(BufferAccessStrategyType btype, int buffers)
> > {
> > BufferAccessStrategy strategy;
> > int ring_size;
> > + const char *strategy_name = btype_get_name(btype);
>
> Shouldn't be executed when we don't need it.
I got rid of it for now.
> > + if (btype != BAS_VACUUM)
> > + {
> > + if (buffers == 0)
> > + elog(ERROR, "Use of shared buffers unsupported for
> > buffer access strategy: %s. nbuffers must be -1.",
> > + strategy_name);
> > +
> > + if (buffers > 0)
> > + elog(ERROR, "Specification of ring size in buffers
> > unsupported for buffer access strategy: %s. nbuffers must be -1.",
> > + strategy_name);
> > + }
> > +
> > + // TODO: DEBUG logging message for dev?
> > + if (buffers == 0)
> > + btype = BAS_NORMAL;
>
> GetAccessStrategy() often can be executed hundreds of thousands of times a
> second, so I'm very sceptical that adding log messages to it useful.
So, in the case of vacuum and autovacuum, I don't see how
GetAccessStrategyExt() could be called hundreds of thousands of times a
second. It is not even called for each table being vacuumed -- it is
only called before vacuuming a list of tables.
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.
So, I've rebelled a bit and implemented a single guc,
buffer_usage_limit, in the attached patchset. Users can set it at the
session or system level or they can specify BUFFER_USAGE_LIMIT to
vacuum. It is the same size for all operations. By default all of this
would be the same as it is now.
The attached patchset does not use the guc for any operations except
VACUUM, though. I will add on another patch if people still feel
strongly that we cannot have a single guc. If the other operations use
this guc, I think we could get much of the same flexibility as having
multiple gucs by just being able to set it at the session level (or
having command options).
> Although I'm not quite opposing the idea of statement level controls
> (like the VACUUM one proposed here), it is better to make these ring
> buffer sizes configurable across the system to help with the other
> similar cases e.g., a CTAS or RMV can help subsequent reads from
> shared buffers if ring buffer is skipped.
Yes, I've done both.
On Tue, Feb 28, 2023 at 3:52 AM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Thu, Feb 23, 2023 at 3:03 AM Melanie Plageman
> 0001:
> 1. I don't quite understand the need for this 0001 patch. Firstly,
> bstrategy allocated once per autovacuum worker in AutovacMemCxt which
> goes away with the process. Secondly, the worker exits after
> do_autovacuum() with which memory context is gone. I think this patch
> is unnecessary unless I'm missing something.
I've dropped this one.
> 0002:
> 1. Don't we need to remove vac_strategy for analyze.c as well? It's
> pretty-meaningless there than vacuum.c as we're passing bstrategy to
> all required functions.
So, it is a bit harder to remove it from analyze because acquire_func
func doesn't take the buffer access strategy as a parameter and
acquire_sample_rows uses the vac_context global variable to pass to
table_scan_analyze_next_block().
We could change acquire_func, but it looks like FDW uses it, so I'm not
sure. It would be more for consistency than as a performance win, as I
imagine analyze is less of a problem than vacuum (i.e. it is probably
reading fewer blocks and probably not dirtying them [unless it does
on-access pruning?]).
I haven't done this in the attached set.
> 0004:
> 1. I think no multiple sentences in a single error message. How about
> "of %d, changing it to %d"?
> + elog(WARNING, "buffer_usage_limit %d is below the
> minimum buffer_usage_limit of %d. setting it to %d",
I've removed this message, but if I put back a message about clamping, I
will remember this note.
> 2. Typically, postgres error messages start with lowercase letters,
> hints and detail messages start with uppercase letters.
> + if (buffers == 0)
> + elog(ERROR, "Use of shared buffers unsupported for buffer
> access strategy: %s. nbuffers must be -1.",
> + strategy_name);
> +
> + if (buffers > 0)
> + elog(ERROR, "Specification of ring size in buffers
> unsupported for buffer access strategy: %s. nbuffers must be -1.",
> + strategy_name);
Thanks! I've removed some of the error messages for now, but, for the
ones I kept, I tthink they are consistent now with this pattern.
> 3. A function for this seems unnecessary, especially when a static
> array would do the needful, something like forkNames[].
> +static const char *
> +btype_get_name(BufferAccessStrategyType btype)
> +{
> + switch (btype)
> + {
I've removed it for now.
>
> 4. Why are these assumptions needed? Can't we simplify by doing
> validations on the new buffers parameter only when the btype is
> BAS_VACUUM?
> + if (buffers == 0)
> + elog(ERROR, "Use of shared buffers unsupported for buffer
> access strategy: %s. nbuffers must be -1.",
> + strategy_name);
>
> + // TODO: DEBUG logging message for dev?
> + if (buffers == 0)
> + btype = BAS_NORMAL;
So, I've moved validation to the vacuum option parsing for the vacuum
option and am using the guc infrastructure to check min and max for the
guc value.
> 5. Is this change needed for this patch?
> default:
> elog(ERROR, "unrecognized buffer access strategy: %d",
> - (int) btype);
> - return NULL; /* keep compiler quiet */
> + (int) btype);
> +
> + pg_unreachable();
The pg_unreachable() is removed, as I've left GetAccessStrategy()
untouched.
- Melanie
From d9a0183b98601e4299b8b12a4ae06e9aaf5c1bfe Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Wed, 22 Feb 2023 15:28:34 -0500
Subject: [PATCH v2 3/3] add vacuum option to specify nbuffers and guc
---
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 | 40 +++++++++++++++
src/backend/utils/init/globals.c | 2 +
src/backend/utils/misc/guc_tables.c | 11 ++++
src/backend/utils/misc/postgresql.conf.sample | 3 ++
src/include/commands/vacuum.h | 1 +
src/include/miscadmin.h | 1 +
src/include/storage/bufmgr.h | 5 ++
11 files changed, 134 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index c62be52e3b..dc51d69821 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -127,6 +127,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)
{
@@ -210,6 +213,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),
@@ -399,7 +439,16 @@ vacuum(List *relations, VacuumParams *params,
{
MemoryContext old_context = MemoryContextSwitchTo(vac_context);
- bstrategy = GetAccessStrategy(BAS_VACUUM);
+ if (params->ring_size == -1)
+ {
+ if (buffer_usage_limit == -1)
+ bstrategy = GetAccessStrategy(BAS_VACUUM);
+ else
+ bstrategy = GetAccessStrategyExt(BAS_VACUUM, buffer_usage_limit);
+ }
+ else
+ bstrategy = GetAccessStrategyExt(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..ffd9308952 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 (buffer_usage_limit == -1)
+ bstrategy = GetAccessStrategy(BAS_VACUUM);
+ else
+ bstrategy = GetAccessStrategyExt(BAS_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 = 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..a7e29f85ac 100644
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -22,6 +22,7 @@
#include "storage/proc.h"
#define INT_ACCESS_ONCE(var) ((int)(*((volatile int *)&(var))))
+#define bufsize_limit_to_nbuffers(bufsize) (bufsize * 1024 / BLCKSZ)
/*
@@ -586,6 +587,45 @@ GetAccessStrategy(BufferAccessStrategyType btype)
return strategy;
}
+
+BufferAccessStrategy
+GetAccessStrategyExt(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 field that didn'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..5167ecddcc 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 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..55d4965d62 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
},
+ {
+ {"buffer_usage_limit", PGC_USERSET, RESOURCES_MEM,
+ gettext_noop("Sets the buffer pool size for operations employing a buffer access strategy."),
+ NULL,
+ GUC_UNIT_KB
+ },
+ &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..d5a8d24621 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -156,6 +156,9 @@
# mmap
# (change requires restart)
#min_dynamic_shared_memory = 0MB # (change requires restart)
+#buffer_usage_limit = -1 # size of buffer access strategy ring. -1 to use default,
+ # 0 to disable buffer access strategy and use shared buffers
+ # > 0 to specify size
# - Disk -
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bdfd96cfec..06bf66f4f4 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -236,6 +236,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..7d7d91b1a4 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 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..32148ba580 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 GetAccessStrategyExt(BufferAccessStrategyType btype, int nbuffers);
extern void FreeAccessStrategy(BufferAccessStrategy strategy);
--
2.37.2
From 615b2881dd1fe9c51acbdf1d1eb83f664205bf48 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Wed, 22 Feb 2023 12:26:01 -0500
Subject: [PATCH v2 2/3] use shared buffers when failsafe active
---
src/backend/access/heap/vacuumlazy.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 8f14cf85f3..b319a244d5 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -2622,6 +2622,11 @@ lazy_check_wraparound_failsafe(LVRelState *vacrel)
if (unlikely(vacuum_xid_failsafe_check(&vacrel->cutoffs)))
{
vacrel->failsafe_active = true;
+ /*
+ * 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
From a4eb134bf003187898bba0fffcaddcb2d964f9af Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Wed, 22 Feb 2023 12:06:41 -0500
Subject: [PATCH v2 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 2e12baf8eb..c62be52e3b 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -74,7 +74,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;
/*
@@ -93,7 +92,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);
@@ -337,7 +336,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.
*/
@@ -403,7 +402,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
@@ -508,7 +506,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;
}
@@ -526,7 +524,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)
{
@@ -1837,7 +1835,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;
@@ -2083,7 +2081,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 */
@@ -2117,7 +2115,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
