v11 attached with updates detailed below. On Tue, Apr 4, 2023 at 11:14 PM David Rowley <dgrowle...@gmail.com> wrote: > > On Wed, 5 Apr 2023 at 05:53, Melanie Plageman <melanieplage...@gmail.com> > wrote: > > Attached v10 addresses the review feedback below. > > Thanks. Here's another round on v10-0001: > > 1. The following documentation fragment does not seem to be aligned > with the code: > > <literal>16 GB</literal>. The minimum size is the lesser > of 1/8 the size of shared buffers and <literal>128 KB</literal>. The > default value is <literal>-1</literal>. If this value is specified > > The relevant code is: > > static inline int > StrategyGetClampedBufsize(int bufsize_kb) > { > int sb_limit_kb; > int blcksz_kb = BLCKSZ / 1024; > > Assert(blcksz_kb > 0); > > sb_limit_kb = NBuffers / 8 * blcksz_kb; > > return Min(sb_limit_kb, bufsize_kb); > } > > The code seems to mean that the *maximum* is the lesser of 16GB and > shared_buffers / 8. You're saying it's the minimum.
Good catch. Fixed. > 2. I think you could get rid of the double "Buffer Access Stategy" in: > > <glossterm linkend="glossary-buffer-access-strategy">Buffer > Access Strategy</glossterm>. > <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 size is > > how about: > > <glossterm linkend="glossary-buffer-access-strategy">Buffer > Access Strategy</glossterm>. > A setting of <literal>0</literal> will allow the operation to use any > number of <varname>shared_buffers</varname>, whereas > <literal>-1</literal> will set the size to a default of > <literal>256 KB</literal>. The maximum size is I've made these changes. > 3. In the following snippet you can use <xref linkend="sql-vacuum"/> > or just <command>VACUUM</command>. There are examples of both in that > file. I don't have a preference as it which, but I think what you've > got isn't great. > > <link linkend="sql-vacuum"><command>VACUUM</command></link> and > <link linkend="sql-analyze"><command>ANALYZE</command></link> I've updated it to use the link. I thought it would be nice to have the link in case the reader wants to look at the BUFFER_USAGE_LIMIT option docs there. > 4. I wonder if there's a reason this needs to be written in the > overview of ANALYZE. > > <command>ANALYZE</command> uses a > <glossterm linkend="glossary-buffer-access-strategy">Buffer Access > Strategy</glossterm> > when reading in the sample data. The number of buffers consumed for this > can > be controlled by <xref linkend="guc-vacuum-buffer-usage-limit"/> or by > using > the <option>BUFFER_USAGE_LIMIT</option> option. > > I think it's fine just to mention it under BUFFER_USAGE_LIMIT. It just > does not seem fundamental enough to be worth being upfront about it. > The other things mentioned in that section don't seem related to > parameters, so there might be no better place for those to go. That's > not the case for what you're adding here. I updated this. > 5. I think I'd rather see the details spelt out here rather than > telling the readers to look at what VACUUM does: > > Specifies the > <glossterm linkend="glossary-buffer-access-strategy">Buffer > Access Strategy</glossterm> > ring buffer size for <command>ANALYZE</command>. See the > <link linkend="sql-vacuum"><command>VACUUM</command></link> option with > the same name. I've updated it to contain the same text, as relevant, as the VACUUM option contains. Note that both rely on the vacuum_buffer_usage_limit GUC documentation for a description of upper and lower bounds. > 6. When I asked about restricting the valid values of > vacuum_buffer_usage_limit to -1 / 0 or 128 KB to 16GB, I didn't expect > you to code in the NBuffers / 8 check. We shouldn't chain > dependencies between GUCs like that. Imagine someone editing their > postgresql.conf after realising shared_buffers is too large for their > RAM, they reduce it and restart. The database fails to start because > vacuum_buffer_usage_limit is too large! Angry DBA? > > Take what's already written about vacuum_failsafe_age as your guidance on > this: > > "The default is 1.6 billion transactions. Although users can set this > value anywhere from zero to 2.1 billion, VACUUM will silently adjust > the effective value to no less than 105% of > autovacuum_freeze_max_age." > > Here we just document the silent capping. You can still claim the > valid range is 128KB to 16GB in the docs. You can mention the 1/8th of > shared buffers cap same as what's mentioned about "105%" above. > > When I mentioned #4 and #10 in my review of the v9-0001 patch, I just > wanted to not surprise users who do vacuum_buffer_usage_limit = 64 and > magically get 128. > 7. In ExecVacuum(), similar to #6 from above, it's also not great that > you're raising an ERROR based on if StrategyGetClampedBufsize() clamps > or not. If someone has a script that does: > > VACUUM (BUFFER_USAGE_LIMIT '1 GB'); it might be annoying that it stops > working when someone adjusts shared buffers from 10GB to 6GB. > > I really think the NBuffers / 8 clamping just should be done inside > GetAccessStrategyWithSize(). Got it. I've done what you suggested. I had some logic issues as well that I fixed and reorderd the code. > 8. I think this ERROR in vacuum.c should mention that 0 is a valid value. > > ereport(ERROR, > (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > errmsg("buffer_usage_limit for a vacuum must be between %d KB and %d KB", > MIN_BAS_RING_SIZE_KB, MAX_BAS_RING_SIZE_KB))); > > I doubt there's a need to mention -1 as that's the same as not > specifying BUFFER_USAGE_LIMIT. I've done this. I didn't say that 0 meant disabling the strategy. Do you think that would be useful? > 9. The following might be worthy of a comment explaining the order of > precedence of how we choose the size: > > if (params->ring_size == -1) > { > if (VacuumBufferUsageLimit == -1) > bstrategy = GetAccessStrategy(BAS_VACUUM); > else > bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, VacuumBufferUsageLimit); > } > else > bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, params->ring_size); I've updated this. Also, after doing so, I realized the if/else logic here and in ExecVacuum() could be better and updated the ordering to more closely mirror the human readable logic. > 10. I wonder if you need to keep bufsize_limit_to_nbuffers(). It's > just used once and seems trivial enough just to write that code inside > GetAccessStrategyWithSize(). I've gotten rid of it. > 11. It's probably worth putting the valid range in the sample config: > > #vacuum_buffer_usage_limit = -1 # size of vacuum and analyze buffer > access strategy ring. > # -1 to use default, > # 0 to disable vacuum buffer access strategy > # > 0 to specify size <-- here Done. > 12. Is bufmgr.h the right place for these? > > /* > * 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. > */ > #define MAX_BAS_RING_SIZE_KB (16 * 1024 * 1024) > #define MIN_BAS_RING_SIZE_KB 128 > > Your comment implies they're VACUUM / ANALYZE limits. If we want to > impose these limits to all access strategies then these seem like good > names and location, otherwise, I imagine miscadmin.h is the correct > place. If so, they'll likely want to be renamed to something more > VACUUM specific. I don't particularly have a preference. 128 - > 1677216 seem like reasonable limits for any buffer access strategy. I don't assert on these limits in GetAccessStrategyWithSize(), and, since the rest of the code is mainly only concerned with vacuum, I think it is better to make these limits vacuum-specific. If we decide to make other access strategies configurable, we can generalize these macros then. As such, I have moved them into miscadmin.h. > 13. I think check_vacuum_buffer_usage_limit() does not belong in > freelist.c. Maybe vacuum.c? I've moved it to vacuum.c. I put it above ExecVacuum() since that would be correct alphabetically, but I'm not sure if it would be better to move it down since ExecVacuum() is the "main entry point". > 14. Not related to this patch, but why do we have half the vacuum > related GUCs in vacuum.c and the other half in globals.c? I see > vacuum_freeze_table_age is defined in vacuum.c but is also needed in > autovacuum.c, so that rules out the globals.c ones being for vacuum.c > and autovacuum.c. It seems a bit messy. I'm not really sure where > VacuumBufferUsageLimit should go now. I've left it where it is and added a (helpful?) comment. > > Remaining TODOs: > > - tests > > - do something about config reload changing GUC > > Shouldn't table_recheck_autovac() pfree/palloc a new strategy if the > size changes? See thoughts about that below in response to Andres' mail. > I'm not sure what the implications are with that and the other patch > you're working on to allow vacuum config changes mid-vacuum. We'll > need to be careful and not immediately break that if that gets > committed then this does or vice-versa. We can think hard about this. If we go with adding a TODO for the size, and keeping the same ring, it won't be a problem. On Wed, Apr 5, 2023 at 1:05 PM Andres Freund <and...@anarazel.de> wrote: > On 2023-04-04 13:53:15 -0400, Melanie Plageman wrote: > > > 8. I don't quite follow this comment: > > > > > > /* > > > * 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? > > > */ > > > > > > Can you explain under what circumstances would vacuum() allocate a > > > bstrategy when do_autovacuum() would not? Is this a case of a config > > > reload where someone changes vacuum_buffer_usage_limit from 0 to > > > something non-zero? If so, perhaps do_autovacuum() needs to detect > > > this and allocate a strategy rather than having vacuum() do it once > > > per table (wastefully). > > Hm. I don't much like that we use a single strategy for multiple tables > today. That way even tiny tables never end up in shared_buffers. But that's > really a discussion for a different thread. However, if were to use a > per-table bstrategy, it'd be a lot easier to react to changes of the config. Agreed. I was wondering if it is okay to do the palloc()/pfree() for every table given that some may be small. > I doubt it's worth adding complications to the code for changing the size of > the ringbuffer during an ongoing vacuum scan, at least for 16. Reacting to > enabling/disbling the ringbuffer alltogether seems a bit more important, but > still not crucial compared to making it configurable at all. > > I think it'd be OK to add a comment saying something like "XXX: In the future > we might want to react to configuration changes of the ring buffer size during > a vacuum" or such. I've added the XXX to the autovacuum code. I think you mean it also could be considered for VACUUM, but I've refrained from mentioning that for now. > WRT to the TODO specifically: Yes, passing in 0 seems to make sense. I don't > see a reason not to do so? But perhaps there's a better solution: I've done that (passed in a 0), I was concerned that future code may reference this vacuum param and expect it to be aligned with the Buffer Access Strategy in use. Really only vacuum() should be referencing the params, so, perhaps it is not an issue... Okay, now I've convinced myself that it is better to allocate the strategy in ExecVacuum(). Then we can get rid of the VacuumParams->ring_size altogether. I haven't done that in this version because of the below concern (re: it being appropriate to allocate the strategy in ExecVacuum() given its current concern/focus). > Perhaps the best solution for autovac vs interactive vacuum issue would be to > move the allocation of the bstrategy to ExecVacuum()? Thought about this -- I did think it might be a bit weird since ExecVacuum() mainly does option handling and sanity checking. Doing Buffer Access Strategy allocation seemed a bit out of place. I've left it as is, but would be happy to change it if the consensus is that this is better. > Random note while looking at the code: > ISTM that adding handling of -1 in GetAccessStrategyWithSize() would make the > code more readable. Instead of > > if (params->ring_size == -1) > { > if (VacuumBufferUsageLimit == -1) > bstrategy = GetAccessStrategy(BAS_VACUUM); > else > bstrategy = > GetAccessStrategyWithSize(BAS_VACUUM, VacuumBufferUsageLimit); > } > else > bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, > params->ring_size); > > you could just have something like: > bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, > params->ring_size != -1 ? params->ring_size : VacuumBufferUsageLimit); > > by falling back to the default values from GetAccessStrategy(). > > Or even more "extremely", you could entirely remove references to > VacuumBufferUsageLimit and handle that in freelist.c Hmm. I see what you mean. I've updated it to this, which is a bit better. if (params->ring_size > -1) bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, params->ring_size); else if (VacuumBufferUsageLimit > -1) bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, VacuumBufferUsageLimit); else bstrategy = GetAccessStrategy(BAS_VACUUM); Not referencing VacuumBufferUsageLimit except in freelist.c is more challenging because I think it would be weird to have GetAccessStrategyWithSize() call GetAccessStrategy() which then calls GetAccessStrategyWithSize(). - Melanie
From 974800478ff6044ea654847fea42413bfd7cea25 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <melanieplage...@gmail.com> Date: Sun, 19 Mar 2023 18:00:28 -0400 Subject: [PATCH v11 2/2] Add buffer-usage-limit option to vacuumdb --- doc/src/sgml/ref/vacuumdb.sgml | 13 +++++++++++++ src/bin/scripts/vacuumdb.c | 23 +++++++++++++++++++++++ src/include/commands/vacuum.h | 3 +++ 3 files changed, 39 insertions(+) diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml index 74bac2d4ba..8280cf0fb0 100644 --- a/doc/src/sgml/ref/vacuumdb.sgml +++ b/doc/src/sgml/ref/vacuumdb.sgml @@ -278,6 +278,19 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry> + <term><option>--buffer-usage-limit <replaceable class="parameter">buffer_usage_limit</replaceable></option></term> + <listitem> + <para> + Specifies the + <glossterm linkend="glossary-buffer-access-strategy">Buffer Access Strategy</glossterm> + ring buffer size for a given invocation of <application>vacuumdb</application>. + This size is used to calculate the number of shared buffers which will + be reused as part of this strategy. See <xref linkend="sql-vacuum"/>. + </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/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c index 39be265b5b..941eac7727 100644 --- a/src/bin/scripts/vacuumdb.c +++ b/src/bin/scripts/vacuumdb.c @@ -46,6 +46,7 @@ typedef struct vacuumingOptions bool process_main; bool process_toast; bool skip_database_stats; + char *buffer_usage_limit; } vacuumingOptions; /* object filter options */ @@ -123,6 +124,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 +149,7 @@ main(int argc, char *argv[]) /* initialize options */ memset(&vacopts, 0, sizeof(vacopts)); vacopts.parallel_workers = -1; + vacopts.buffer_usage_limit = NULL; vacopts.no_index_cleanup = false; vacopts.force_index_cleanup = false; vacopts.do_truncate = true; @@ -266,6 +269,9 @@ main(int argc, char *argv[]) case 12: vacopts.process_main = false; break; + case 13: + vacopts.buffer_usage_limit = pg_strdup(optarg); + break; default: /* getopt_long already emitted a complaint */ pg_log_error_hint("Try \"%s --help\" for more information.", progname); @@ -343,6 +349,11 @@ main(int argc, char *argv[]) pg_fatal("cannot use the \"%s\" option with the \"%s\" option", "no-index-cleanup", "force-index-cleanup"); + /* buffer-usage-limit doesn't make sense with VACUUM FULL */ + if (vacopts.buffer_usage_limit && vacopts.full) + pg_fatal("cannot use the \"%s\" option with the \"%s\" option", + "buffer-usage-limit", "full"); + /* fill cparams except for dbname, which is set below */ cparams.pghost = host; cparams.pgport = port; @@ -550,6 +561,10 @@ vacuum_one_database(ConnParams *cparams, pg_fatal("cannot use the \"%s\" option on server versions older than PostgreSQL %s", "--parallel", "13"); + if (vacopts->buffer_usage_limit && 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 +1063,13 @@ prepare_vacuum_command(PQExpBuffer sql, int serverVersion, vacopts->parallel_workers); sep = comma; } + if (vacopts->buffer_usage_limit) + { + Assert(serverVersion >= 160000); + appendPQExpBuffer(sql, "%sBUFFER_USAGE_LIMIT '%s'", sep, + vacopts->buffer_usage_limit); + sep = comma; + } if (sep != paren) appendPQExpBufferChar(sql, ')'); } @@ -1111,6 +1133,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 size of ring buffer 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 2ee5147e97..f027d3ed71 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 { -- 2.37.2
From b658f5bed15e02bff41fb89ac7b270bd187412da Mon Sep 17 00:00:00 2001 From: Melanie Plageman <melanieplage...@gmail.com> Date: Wed, 5 Apr 2023 15:18:50 -0400 Subject: [PATCH v11 1/2] Add VACUUM BUFFER_USAGE_LIMIT option and GUC Add GUC, vacuum_buffer_usage_limit, and VACUUM option, BUFFER_USAGE_LIMIT, through which the user can specify the maximum size to use for buffers for VACUUM, ANALYZE, and autovacuum. The size is converted into a number of shared buffers which are tracked in the BufferAccessStrategyData object. The explicit VACUUM option, when specified, overrides the GUC value -- unless it is specified as -1. Reviewed-by: David Rowley <dgrowle...@gmail.com> Reviewed-by: Andres Freund <and...@anarazel.de> Reviewed-by: Justin Pryzby <pry...@telsasoft.com> Discussion: https://www.postgresql.org/message-id/flat/20230111182720.ejifsclfwymw2reb%40awork3.anarazel.de --- doc/src/sgml/config.sgml | 30 ++++++ doc/src/sgml/ref/analyze.sgml | 18 ++++ doc/src/sgml/ref/vacuum.sgml | 22 +++++ src/backend/commands/vacuum.c | 96 ++++++++++++++++++- src/backend/commands/vacuumparallel.c | 14 ++- src/backend/postmaster/autovacuum.c | 22 ++++- src/backend/storage/buffer/README | 4 + src/backend/storage/buffer/freelist.c | 48 ++++++++-- src/backend/utils/init/globals.c | 5 +- src/backend/utils/misc/guc_tables.c | 11 +++ src/backend/utils/misc/postgresql.conf.sample | 4 + src/bin/psql/tab-complete.c | 4 +- src/include/commands/vacuum.h | 6 ++ src/include/miscadmin.h | 13 +++ src/include/storage/bufmgr.h | 6 ++ src/include/utils/guc_hooks.h | 3 + src/test/regress/expected/vacuum.out | 4 + src/test/regress/sql/vacuum.sql | 3 + 18 files changed, 299 insertions(+), 14 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index bcc49aec45..1d75ec62df 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -2001,6 +2001,36 @@ 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. This size is converted to the number of shared buffers + which will be reused as part of a + <glossterm linkend="glossary-buffer-access-strategy">Buffer Access Strategy</glossterm>. + A setting of <literal>0</literal> will allow the operation to use any + number of <varname>shared_buffers</varname>, whereas + <literal>-1</literal> will set the size to a default of <literal>256 KB</literal>. + The maximum size is <literal>16 GB</literal> and the minimum size is <literal>128 KB</literal>. + If the specified size would exceed 1/8 the size of + <varname>shared_buffers</varname>, it is silently capped. + The default value is <literal>-1</literal>. If this value is specified + without units, it is taken as kilobytes. This parameter can be set at + any time. It can be overridden for + <xref linkend="sql-vacuum"/> and <xref linkend="sql-analyze"/> + when passing the <option>BUFFER_USAGE_LIMIT</option> option. + </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/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 2f94e89cb0..16cde8668e 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -28,6 +28,7 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea VERBOSE [ <replaceable class="parameter">boolean</replaceable> ] SKIP_LOCKED [ <replaceable class="parameter">boolean</replaceable> ] + BUFFER_USAGE_LIMIT [ <replaceable class="parameter">string</replaceable> ] <phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase> @@ -95,6 +96,23 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea </listitem> </varlistentry> + <varlistentry> + <term><literal>BUFFER_USAGE_LIMIT</literal></term> + <listitem> + <para> + Specifies the + <glossterm linkend="glossary-buffer-access-strategy">Buffer Access Strategy</glossterm> + ring buffer size for <command>ANALYZE</command>. This size is used to + calculate the number of shared buffers which will be reused as part of + this strategy. <literal>0</literal> disables use of a + <literal>Buffer Access Strategy</literal>. <literal>-1</literal> + indicates that <command>ANALYZE</command> should fall back to the value + specified by <xref linkend="guc-vacuum-buffer-usage-limit"/>. + </para> + </listitem> + </varlistentry> + + <varlistentry> <term><replaceable class="parameter">boolean</replaceable></term> <listitem> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index b6d30b5764..c1ddf88cf8 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">string</replaceable> ] <phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase> @@ -345,6 +346,27 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet </listitem> </varlistentry> + <varlistentry> + <term><literal>BUFFER_USAGE_LIMIT</literal></term> + <listitem> + <para> + Specifies the + <glossterm linkend="glossary-buffer-access-strategy">Buffer Access Strategy</glossterm> + ring buffer size for <command>VACUUM</command>. This size is used to + calculate the number of shared buffers which will be reused as part of + this strategy. <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"/>. + This option can't be used with the <option>FULL</option> option or + <option>ONLY_DATABASE_STATS</option> option. If + <option>ANALYZE</option> is also specified, the + <option>BUFFER_USAGE_LIMIT</option> value is used for both the vacuum + and analyze stages. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><replaceable class="parameter">boolean</replaceable></term> <listitem> diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index da85330ef4..1027af095b 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,29 @@ 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. This should match the checking in ExecVacuum(). + */ +bool +check_vacuum_buffer_usage_limit(int *newval, void **extra, + GucSource source) +{ + /* Allow specifying the default or disabling Buffer Access Strategy */ + if (*newval == -1 || *newval == 0) + return true; + + /* Value upper and lower hard limits are inclusive */ + if (*newval >= MIN_BAS_VAC_RING_SIZE_KB && *newval <= MAX_BAS_VAC_RING_SIZE_KB) + return true; + + /* Value does not fall within any allowable range */ + GUC_check_errdetail("\"vacuum_buffer_usage_limit\" must be 0 or between %d KB and %d KB", + MIN_BAS_VAC_RING_SIZE_KB, MAX_BAS_VAC_RING_SIZE_KB); + + return false; +} + /* * Primary entry point for manual VACUUM and ANALYZE commands * @@ -124,6 +148,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) { @@ -134,6 +161,46 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) verbose = defGetBoolean(opt); else if (strcmp(opt->defname, "skip_locked") == 0) skip_locked = defGetBoolean(opt); + else if (strcmp(opt->defname, "buffer_usage_limit") == 0) + { + const char *hintmsg; + int result; + char *vac_buffer_size; + + 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, &hintmsg)) + { + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("value: \"%s\": is invalid for buffer_usage_limit", + vac_buffer_size), + hintmsg ? errhint("%s", _(hintmsg)) : 0)); + } + + /* + * Check that the specified size falls within the hard upper and + * lower limits if it is not -1 or 0. + */ + if (result != -1 && result != 0 && + (result < MIN_BAS_VAC_RING_SIZE_KB || result > MAX_BAS_VAC_RING_SIZE_KB)) + { + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("buffer_usage_limit for a vacuum must be 0 or between %d KB and %d KB", + MIN_BAS_VAC_RING_SIZE_KB, MAX_BAS_VAC_RING_SIZE_KB))); + } + + params.ring_size = result; + } else if (!vacstmt->is_vacuumcmd) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -238,6 +305,16 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("VACUUM FULL cannot be performed in parallel"))); + if ((params.options & VACOPT_FULL) && params.ring_size > -1) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("BUFFER_USAGE_LIMIT cannot be specified for VACUUM FULL"))); + + if ((params.options & VACOPT_ONLY_DATABASE_STATS) && params.ring_size > -1) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("BUFFER_USAGE_LIMIT cannot be specified for VACUUM ONLY_DATABASE_STATS"))); + /* * Make sure VACOPT_ANALYZE is specified if any column lists are present. */ @@ -401,7 +478,24 @@ vacuum(List *relations, VacuumParams *params, { MemoryContext old_context = MemoryContextSwitchTo(vac_context); - bstrategy = GetAccessStrategy(BAS_VACUUM); + Assert(params->ring_size >= -1); + + /* + * If explicit VACUUM or ANALYZE specified a non-default + * BUFFER_USAGE_LIMIT, that overrides the value of GUC + * VacuumBufferUsageLimit. If VacuumBufferUsageLimit is set to + * something other than the default, use its value. Otherwise, use the + * default sizes for a Buffer Access Strategy. Note that autovacuum + * will have set VacuumParams->ring_size to the value it derived from + * VacuumBufferUsageLimit. + */ + if (params->ring_size > -1) + bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, params->ring_size); + else if (VacuumBufferUsageLimit > -1) + bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, VacuumBufferUsageLimit); + else + bstrategy = GetAccessStrategy(BAS_VACUUM); + MemoryContextSwitchTo(old_context); } diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c index 563117a8f6..614a35779a 100644 --- a/src/backend/commands/vacuumparallel.c +++ b/src/backend/commands/vacuumparallel.c @@ -87,6 +87,12 @@ typedef struct PVShared */ int maintenance_work_mem_worker; + /* + * The number of buffers each worker's Buffer Access Strategy ring should + * contain. + */ + int ring_nbuffers; + /* * Shared vacuum cost balance. During parallel vacuum, * VacuumSharedCostBalance points to this value and it accumulates the @@ -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 */ + shared->ring_nbuffers = StrategyGetBufferCount(bstrategy); + pg_atomic_init_u32(&(shared->cost_balance), 0); pg_atomic_init_u32(&(shared->active_nworkers), 0); pg_atomic_init_u32(&(shared->idx), 0); @@ -1018,8 +1027,9 @@ 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 */ - pvs.bstrategy = GetAccessStrategy(BAS_VACUUM); + /* Each parallel VACUUM worker gets its own access strategy. */ + pvs.bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, + shared->ring_nbuffers * (BLCKSZ / 1024)); /* Setup error traceback support for ereport() */ errcallback.callback = parallel_vacuum_error_callback; diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 585d28148c..729a4c144a 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -2290,9 +2290,18 @@ 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. + * 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. + * + * XXX: In the future we may want to react to changes in + * VacuumBufferUsageLimit while vacuuming. */ - bstrategy = GetAccessStrategy(BAS_VACUUM); + if (VacuumBufferUsageLimit > -1) + bstrategy = GetAccessStrategyWithSize(BAS_VACUUM, VacuumBufferUsageLimit); + else + bstrategy = GetAccessStrategy(BAS_VACUUM); /* * create a memory context to act as fake PortalContext, so that the @@ -2884,6 +2893,15 @@ 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; + + /* + * Since we currently do not change the size of the Buffer Access + * Strategy while the autovacuum worker is running, always set this to + * 0 to avoid changes in VacuumBufferUsageLimit affecting allocation + * of the Buffer Usage Strategy. + */ + tab->at_params.ring_size = 0; + 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..d7c9d99af2 100644 --- a/src/backend/storage/buffer/README +++ b/src/backend/storage/buffer/README @@ -236,6 +236,10 @@ buffers were sent to the freelist, which was effectively a buffer ring of 1 buffer, resulting in excessive WAL flushing. Allowing VACUUM to update 256KB between WAL flushes should be more efficient. +In v16, the 256KB ring was made configurable by way of the +vacuum_buffer_usage_limit GUC and the BUFFER_USAGE_LIMIT option to VACUUM and +ANALYZE. + Bulk writes work similarly to VACUUM. Currently this applies only to COPY IN and CREATE TABLE AS SELECT. (Might it be interesting to make seqscan UPDATE and DELETE use the bulkwrite strategy?) For bulk writes diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index f122709fbe..0c1ce8b93f 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -540,8 +540,7 @@ StrategyInitialize(bool init) BufferAccessStrategy GetAccessStrategy(BufferAccessStrategyType btype) { - BufferAccessStrategy strategy; - int nbuffers; + int ring_size_kb; /* * Select ring size to use. See buffer/README for rationales. @@ -556,13 +555,13 @@ GetAccessStrategy(BufferAccessStrategyType btype) return NULL; case BAS_BULKREAD: - nbuffers = 256 * 1024 / BLCKSZ; + ring_size_kb = 256; break; case BAS_BULKWRITE: - nbuffers = 16 * 1024 * 1024 / BLCKSZ; + ring_size_kb = 16 * 1024; break; case BAS_VACUUM: - nbuffers = 256 * 1024 / BLCKSZ; + ring_size_kb = 256; break; default: @@ -571,8 +570,36 @@ GetAccessStrategy(BufferAccessStrategyType btype) return NULL; /* keep compiler quiet */ } + return GetAccessStrategyWithSize(btype, ring_size_kb); +} + +/* + * GetAccessStrategyWithSize -- create a BufferAccessStrategy object with a + * number of buffers equivalent to the passed in size + */ +BufferAccessStrategy +GetAccessStrategyWithSize(BufferAccessStrategyType btype, int ring_size_kb) +{ + int blcksz_kb = BLCKSZ / 1024; + int nbuffers; + int sb_limit_kb; + BufferAccessStrategy strategy; + + /* Default nbuffers should have resulted in calling GetAccessStrategy() */ + Assert(ring_size_kb >= 0); + + if (ring_size_kb == 0) + return NULL; + + Assert(blcksz_kb > 0); + /* Make sure ring isn't an undue fraction of shared buffers */ - nbuffers = Min(NBuffers / 8, nbuffers); + sb_limit_kb = NBuffers / 8 * blcksz_kb; + ring_size_kb = Min(sb_limit_kb, ring_size_kb); + + nbuffers = ring_size_kb / blcksz_kb; + + Assert(nbuffers > 0); /* Allocate the object and initialize all elements to zeroes */ strategy = (BufferAccessStrategy) @@ -586,6 +613,15 @@ GetAccessStrategy(BufferAccessStrategyType btype) return strategy; } +/* + * StrategyGetBufferCount -- an accessor for the number of buffers in the ring + */ +int +StrategyGetBufferCount(BufferAccessStrategy strategy) +{ + return strategy->nbuffers; +} + /* * FreeAccessStrategy -- release a BufferAccessStrategy object * diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 1b1d814254..059a097bef 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -139,7 +139,10 @@ int max_worker_processes = 8; int max_parallel_workers = 8; int MaxBackends = 0; -int VacuumCostPageHit = 1; /* GUC parameters for vacuum */ +/* GUC parameters for vacuum */ +int VacuumBufferUsageLimit = -1; + +int VacuumCostPageHit = 1; int VacuumCostPageMiss = 2; int VacuumCostPageDirty = 20; int VacuumCostLimit = 200; diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 8062589efd..5bad2b540f 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -2224,6 +2224,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, ANALYZE, and autovacuum."), + NULL, + GUC_UNIT_KB + }, + &VacuumBufferUsageLimit, + -1, -1, MAX_BAS_VAC_RING_SIZE_KB, + check_vacuum_buffer_usage_limit, 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 ee49ca3937..cdbcb95a07 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -157,6 +157,10 @@ # mmap # (change requires restart) #min_dynamic_shared_memory = 0MB # (change requires restart) +#vacuum_buffer_usage_limit = -1 # size of vacuum and analyze buffer access strategy ring. + # -1 to use default, + # 0 to disable vacuum buffer access strategy + # > 0 to specify size. range 128-16777216 # - Disk - diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index e38a49e8bd..6614fd2e62 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -2662,7 +2662,7 @@ psql_completion(const char *text, int start, int end) * one word, so the above test is correct. */ if (ends_with(prev_wd, '(') || ends_with(prev_wd, ',')) - COMPLETE_WITH("VERBOSE", "SKIP_LOCKED"); + COMPLETE_WITH("VERBOSE", "SKIP_LOCKED", "BUFFER_USAGE_LIMIT"); else if (TailMatches("VERBOSE|SKIP_LOCKED")) COMPLETE_WITH("ON", "OFF"); } @@ -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/include/commands/vacuum.h b/src/include/commands/vacuum.h index bdfd96cfec..2ee5147e97 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -236,6 +236,12 @@ typedef struct VacuumParams * disabled. */ int nworkers; + + /* + * The size in KB of the Buffer Access Strategy ring to be used for VACUUM + * and ANALYZE. + */ + int ring_size; } VacuumParams; /* diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 06a86f9ac1..2d550f26e0 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -263,6 +263,19 @@ extern PGDLLIMPORT double hash_mem_multiplier; extern PGDLLIMPORT int maintenance_work_mem; extern PGDLLIMPORT int max_parallel_maintenance_workers; +/* + * Global values used by VACUUM and autovacuum. + */ +extern PGDLLIMPORT int VacuumBufferUsageLimit; + +/* + * 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. + */ +#define MAX_BAS_VAC_RING_SIZE_KB (16 * 1024 * 1024) +#define MIN_BAS_VAC_RING_SIZE_KB 128 + extern PGDLLIMPORT int VacuumCostPageHit; extern PGDLLIMPORT int VacuumCostPageMiss; extern PGDLLIMPORT int VacuumCostPageDirty; diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h index 73762cb1ec..c174cb2c04 100644 --- a/src/include/storage/bufmgr.h +++ b/src/include/storage/bufmgr.h @@ -194,7 +194,13 @@ extern Size BufferShmemSize(void); extern void AtProcExit_LocalBuffers(void); /* in freelist.c */ + extern BufferAccessStrategy GetAccessStrategy(BufferAccessStrategyType btype); +extern BufferAccessStrategy GetAccessStrategyWithSize( + BufferAccessStrategyType btype, + int ring_size_kb); +extern int StrategyGetBufferCount(BufferAccessStrategy strategy); + extern void FreeAccessStrategy(BufferAccessStrategy strategy); diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h index aeb3663071..1893292634 100644 --- a/src/include/utils/guc_hooks.h +++ b/src/include/utils/guc_hooks.h @@ -33,6 +33,9 @@ extern bool check_autovacuum_max_workers(int *newval, void **extra, GucSource source); extern bool check_autovacuum_work_mem(int *newval, void **extra, GucSource source); + +extern bool check_vacuum_buffer_usage_limit(int *newval, void **extra, + GucSource source); extern bool check_backtrace_functions(char **newval, void **extra, GucSource source); extern void assign_backtrace_functions(const char *newval, void *extra); diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index e5a312182e..fe675c5a2e 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -350,6 +350,10 @@ SELECT t.relfilenode = :toast_filenode AS is_same_toast_filenode f (1 row) +-- BUFFER_USAGE_LIMIT integer overflow error +VACUUM (BUFFER_USAGE_LIMIT 10000000000) vac_option_tab; +ERROR: value: "10000000000": is invalid for buffer_usage_limit +HINT: Value exceeds integer range. -- SKIP_DATABASE_STATS option VACUUM (SKIP_DATABASE_STATS) vactst; -- ONLY_DATABASE_STATS option diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql index a1fad43657..fc068b7705 100644 --- a/src/test/regress/sql/vacuum.sql +++ b/src/test/regress/sql/vacuum.sql @@ -272,6 +272,9 @@ SELECT t.relfilenode = :toast_filenode AS is_same_toast_filenode FROM pg_class c, pg_class t WHERE c.reltoastrelid = t.oid AND c.relname = 'vac_option_tab'; +-- BUFFER_USAGE_LIMIT integer overflow error +VACUUM (BUFFER_USAGE_LIMIT 10000000000) vac_option_tab; + -- SKIP_DATABASE_STATS option VACUUM (SKIP_DATABASE_STATS) vactst; -- 2.37.2