From: Justin Pryzby <pryzby@telsasoft.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: pgsql-hackers@postgresql.org, Robert Haas <robertmhaas@gmail.com>
Cc: David Rowley <dgrowleyml@gmail.com>
Subject: Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
Date: Sat, 18 Mar 2023 13:30:38 -0500
Message-ID: <ZBYDTrD1kyGg+HkS@telsasoft.com> (raw)
In-Reply-To: <CAAKRu_ZqkqN0g+kRenjtHruDjYNrdK8boaeq9pA1+=oVO+2izA@mail.gmail.com>
References: <20230111191842.5ywh7twkis42kxad@awork3.anarazel.de>
<CAH2-Wz=Ur6J+A2mXB1wZm-QmZWTMWiVk0sn4f-0mbQTyFXQhHg@mail.gmail.com>
<669645.1673471914@sss.pgh.pa.us>
<20230111213905.4ljixbm2oyqzvzzx@awork3.anarazel.de>
<CAAKRu_b1q_07uquUtAvLqTM=W9nzee7QbtzHwA4XdUo7KX_Cnw@mail.gmail.com>
<CALj2ACXKgAQpKsCPi6ox+K5JLDB9TAxeObyVOfrmgTjqmc0aAA@mail.gmail.com>
<CAAKRu_brtqmd4e7kwEeKjySP22y4ywF32M7pvpi+x5txgF0+ig@mail.gmail.com>
<CAAKRu_ZEPLwTL==L5jurkbr-wXK+-n+sph5-hFJ9wkNRBoH-dw@mail.gmail.com>
<CAD21AoCAXwhgvSx-BXhor7gkm+0HwCHvdwtg-DS4N7UFyzvn6Q@mail.gmail.com>
<CAAKRu_ZqkqN0g+kRenjtHruDjYNrdK8boaeq9pA1+=oVO+2izA@mail.gmail.com>
> Subject: [PATCH v4 3/3] add vacuum[db] option to specify ring_size and guc
> + 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
I'd say "specifies the size of shared_buffers to be reused as .."
> + 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.
GUC docs usually also say something like
"If this value is specified without units, it is taken as .."
> + is used to calculate a number of shared buffers which will be reused as
*the* number?
> + <command>VACUUM</command>. The analyze stage and parallel vacuum workers
> + do not use this size.
I think what you mean is that vacuum's heap scan stage uses the
strategy, but the index scan/cleanup phases doesn't?
> + 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
*the* number
> +++ b/doc/src/sgml/ref/vacuumdb.sgml
The docs here duplicate the sql-vacuum docs. It seems better to refer
to the vacuum page for details, like --parallel does.
Unrelated: it would be nice if the client-side options were documented
separately from the server-side options. Especially due to --jobs and
--parallel.
> + 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)));
> + }
I think these checks could be collapsed into a single ereport().
if !parse_int() || (result < -1 || result > MAX_BAS_RINGSIZE_KB):
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("buffer_usage_limit for a vacuum must be an integer between -1 and %d",
MAX_BAS_RING_SIZE_KB),
There was a recent, similar, and unrelated suggestion here:
https://www.postgresql.org/message-id/20230314.135859.260879647537075548.horikyota.ntt%40gmail.com> +#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
I think it's confusing to say "and use shared buffers", since
"strategies" also use shared_buffers. It seems better to remove those 4
words.
> @@ -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.
Actually, that seems fine to me. If someone installs v16 vacuumdb, they
can run it against old servers and specify the option as -1 without it
failing with an error. I don't know if anyone will find that useful,
but it doesn't seem unreasonable.
I still think adding something to the glossary would be good.
Buffer Access Strategy:
A circular/ring buffer used for reading or writing data pages from/to
the operating system. Ring buffers are used for sequential scans of
large tables, VACUUM, COPY FROM, CREATE TABLE AS SELECT, ALTER TABLE,
and CLUSTER. By using only a limited portion of >shared_buffers<, the
ring buffer avoids avoids evicting large amounts of data whenever a
backend performs bulk I/O operations. Use of a ring buffer also forces
the backend to write out its own dirty pages, rather than leaving them
behind to be cleaned up by other backends.
If there's a larger section added than a glossary entry, the text could
be promoted from src/backend/storage/buffer/README to doc/.
--
Justin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, melanieplageman@gmail.com, sawada.mshk@gmail.com, bharath.rupireddyforpostgres@gmail.com, andres@anarazel.de, tgl@sss.pgh.pa.us, pg@bowt.ie, robertmhaas@gmail.com, dgrowleyml@gmail.com
Subject: Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
In-Reply-To: <ZBYDTrD1kyGg+HkS@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox