Feedback-ID: i131946ab:Fastmail
Message-ID: <16845cb1-b228-e157-f293-5892bced9253@enterprisedb.com>
Date: Fri, 14 Apr 2023 09:20:41 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.9.1
Subject: Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
Content-Language: en-US
To: David Rowley <dgrowleyml@gmail.com>, Andres Freund <andres@anarazel.de>,
 Justin Pryzby <pryzby@telsasoft.com>, Masahiko Sawada
 <sawada.mshk@gmail.com>,
 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
 Tom Lane <tgl@sss.pgh.pa.us>, Peter Geoghegan <pg@bowt.ie>,
 pgsql-hackers@postgresql.org, Robert Haas <robertmhaas@gmail.com>
References: <CAAKRu_ZevZBtvqM1Eh3Wr587wf2vCW=XxOkaUn=s12apE5ekZg@mail.gmail.com>
 <20230405211534.4skgskbilnxqrmxg@awork3.anarazel.de>
 <CAAKRu_bJRKe+v_=OqwC+5sA3j5qv8rqdAwy3+yHaO3wmtfrCRg@mail.gmail.com>
 <CAAKRu_byOPANPzNiu=d2ACvfzWfNURPYPwo2v7iWddHLFLMcJA@mail.gmail.com>
 <CAApHDvo57+sYWx+nwM3DXM2m0S8f1XDruTURSWjdwOSRKu6s9Q@mail.gmail.com>
 <CAAKRu_bwE+kov=1Y35-dxF4jandvFWA1NiSyCG_=kUFs9xvw-Q@mail.gmail.com>
 <CAAKRu_YdzP42WTZPQO0QePosxFqjHavC1v87BMaaiXqD9b12wA@mail.gmail.com>
 <CAApHDvrDsCeyVxkkrfe5H4gsw4sBDSPL8cK2fAdhMEn+pY96Rw@mail.gmail.com>
 <20230406172056.gnp7ohxs4bykrn5z@liskov>
 <CAApHDvqhdpN6qdy4pTpt5FWm6A1M4O37fT+r7819t5OJJ4tfUg@mail.gmail.com>
 <20230406214431.2pkmcogizdamdmzz@liskov>
 <CAApHDvqs+Fcw9Yrn4ORCAX0xKK1-SiCC0w1j7Dhg=G9hrvag7g@mail.gmail.com>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <CAApHDvqs+Fcw9Yrn4ORCAX0xKK1-SiCC0w1j7Dhg=G9hrvag7g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/16845cb1-b228-e157-f293-5892bced9253%40enterprisedb.com>
Precedence: bulk

On 07.04.23 02:52, David Rowley wrote:
> On Fri, 7 Apr 2023 at 09:44, Melanie Plageman <melanieplageman@gmail.com> wrote:
>> Otherwise, LGTM.
> 
> Thanks for looking.  I've also taken Justin's comments about the
> README into account and fixed that part.
> 
> I've pushed the patch after a little more adjusting.  I added some
> text to the docs that mention larger VACUUM_BUFFER_LIMITs can speed up
> vacuum and also a reason why they might not want to go nuts with it.

I came across these new options and had a little bit of trouble figuring 
them out from the documentation.  Maybe this could be polished a bit.

vacuumdb --help says

     --buffer-usage-limit=BUFSIZE

I can guess what a "SIZE" might be, but is "BUFSIZE" different from a 
"SIZE"?  Maybe simplify here.

On the vacuumdb man page, the placeholder is

     <replaceable class="parameter">buffer_usage_limit</replaceable>

which is yet another way of phrasing it.  Maybe also use "size" here?

The VACUUM man page says

     BUFFER_USAGE_LIMIT [ <replaceable ...>string</replaceable> ]

which had me really confused.  The detailed description later doesn't 
give any further explanation of possible values, except that 
<literal>0</literal> is apparently a possible value, which in my mind is 
not a string.  Then there is a link to guc-vacuum-buffer-usage-limit, 
which lifts the mystery that this is really just an integer setting with 
possible memory-size units, but it was really hard to figure that out 
from the start!

Moreover, on the VACUUM man page, right below BUFFER_USAGE_LIMIT, it 
explains the different kinds of accepted values, and "string" wasn't 
added there.  Maybe also change this to "size" here and add an 
explanation there what kinds of sizes are possible.

Finally, the locations of the new options in the various documentation 
places seems a bit random.  The vacuumdb --help output and the man page 
appear to be mostly alphabetical, so --buffer-usage-limit should be 
after -a/--all.  (Also note that right now the option isn't even in the 
same place in the --help output versus the man page.)

The order of the options on the VACUUM man page doesn't make any sense 
anymore.  This isn't really the fault of this patch, but maybe it's time 
to do a fresh reordering there.