Date: Sat, 1 Apr 2023 12:57:23 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: David Rowley <dgrowleyml@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>,
	Peter Geoghegan <pg@bowt.ie>, pgsql-hackers@postgresql.org,
	Robert Haas <robertmhaas@gmail.com>
Subject: Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
Message-ID: <ZChwg92pn4P2Id4n@telsasoft.com>
References: 
 <CAAKRu_ZqkqN0g+kRenjtHruDjYNrdK8boaeq9pA1+=oVO+2izA@mail.gmail.com>
 <ZBYDTrD1kyGg+HkS@telsasoft.com>
 <CAAKRu_YefVbhg4rAxU2frYFdTap61UftH-kUNQZBvAs+YK81Zg@mail.gmail.com>
 <CAApHDvq9a_GCuF_1YV_a8t+JW3cesuAxWNvONWCXsChdefD42g@mail.gmail.com>
 <CAAKRu_Y81TuNi3PpeoDvqoDVM1JYM07CPiOUgThY2MU50OcZVg@mail.gmail.com>
 <CAApHDvqzik7UTOJPQhMu3nEw5GJpHSAgUNhw05hcELx3Kqx=pg@mail.gmail.com>
 <CAAKRu_Yc3QHPfwhCJLjvAW0HhePLB2GHzFTEcs9O+G3U_VrKaQ@mail.gmail.com>
 <CAApHDvqk_0j1ewnq8Xu+BKhOj76EXO=ejBXocdvyRkgyPV8ehw@mail.gmail.com>
 <CAAKRu_Zbrp=O+pzmo3UXe2LWfMOLf9QHjOsh59fk9x7NLG1oQA@mail.gmail.com>
 <CAAKRu_bRTy-sCUuFz3HYhz2Z==XArFJCQwmvJaDVFTJUpZKWCg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAAKRu_bRTy-sCUuFz3HYhz2Z==XArFJCQwmvJaDVFTJUpZKWCg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZChwg92pn4P2Id4n%40telsasoft.com>
Precedence: bulk

On Sat, Apr 01, 2023 at 01:29:13PM -0400, Melanie Plageman wrote:
> Hi,
> 
> I was just doing some cleanup on the main patch in this set and realized
> that it was missing a few things. One of which is forbidding the
> BUFFER_USAGE_LIMIT with VACUUM FULL since VACUUM FULL does not use a
> BAS_VACUUM strategy.
> 
> VACUUM FULL technically uses a bulkread buffer access strategy for
> reading the original relation if its number of blocks is > number of
> shared buffers / 4 (see initscan()). The new rel writing is done using
> smgrextend/write directly and doesn't go through shared buffers. I
> think it is a stretch to try and use the size passed in to VACUUM by
> BUFFER_USAGE_LIMIT for the bulkread strategy ring.

When you say that it's a stretch, do you mean that it'd be a pain to add
arguments to handful of functions to pass down the setting ?  Or that
it's unclear if doing so would be the desirable/needed/intended/expected
behavior ?

I think if VACUUM FULL were going to allow a configurable strategy size,
then so should CLUSTER.  But it seems fine if they don't.

I wonder if maybe strategy should be configurable in some more generic
way, like a GUC.  At one point I had a patch to allow INSERT to use
strategy buffers (not just INSERT SELECT).  And that's still pretty
desirable.  Also COPY.  I've seen load spikes caused by pg_dumping
tables which are just below 25% of shared_buffers.  Which is exacerbated
because pg_dump deliberately orders tables by size, so those tables are
dumped one after another, each causing eviction of ~20% of shared
buffers.  And exacerbated some more because TOAST don't seem to use a
ring buffer in that case.

> I somehow feel like VACUUM (FULL, BUFFER_USAGE_LIMIT 'x') should error
> out instead of silently not using the buffer usage limit, though.
> 
> I am looking for others' opinions.

Sorry, no opinion here :)

One thing is that it's fine to take something that previously throw an
error and change it to not throw an error anymore.  But it's undesirable
to do the opposite.  For that reason, there's may be a tendency to add
errors for cases like this.

-- 
Justin