Date: Sat, 11 Mar 2023 13:16:19 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: 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>,
	David Rowley <dgrowleyml@gmail.com>
Subject: Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
Message-ID: <ZAzTg3iEnubscvbf@telsasoft.com>
References: <20230111185854.5srlvequbxbg5p6c@awork3.anarazel.de>
 <CAH2-Wzkwr2E0KvLtaFWUxMp8qow5nqUQMSQsVUa3KLabXJQPyA@mail.gmail.com>
 <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAAKRu_ZEPLwTL==L5jurkbr-wXK+-n+sph5-hFJ9wkNRBoH-dw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZAzTg3iEnubscvbf%40telsasoft.com>
Precedence: bulk

On Sat, Mar 11, 2023 at 09:55:33AM -0500, Melanie Plageman wrote:
> Subject: [PATCH v3 2/3] use shared buffers when failsafe active
> 
> +		/*
> +		 * Assume the caller who allocated the memory for the
> +		 * BufferAccessStrategy object will free it.
> +		 */
> +		vacrel->bstrategy = NULL;

This comment could use elaboration:

** VACUUM normally restricts itself to a small ring buffer; but in
failsafe mode, in order to process tables as quickly as possible, allow
the leaving behind large number of dirty buffers.

> Subject: [PATCH v3 3/3] add vacuum option to specify ring_size and guc

>  #define INT_ACCESS_ONCE(var)	((int)(*((volatile int *)&(var))))
> +#define bufsize_limit_to_nbuffers(bufsize) (bufsize * 1024 / BLCKSZ)

Macros are normally be capitalized

It's a good idea to write "(bufsize)", in case someone passes "a+b".

> @@ -586,6 +587,45 @@ GetAccessStrategy(BufferAccessStrategyType btype)
> +BufferAccessStrategy
> +GetAccessStrategyWithSize(BufferAccessStrategyType btype, int ring_size)

Maybe it would make sense for GetAccessStrategy() to call
GetAccessStrategyWithSize().  Or maybe not.

> +		{"vacuum_buffer_usage_limit", PGC_USERSET, RESOURCES_MEM,
> +			gettext_noop("Sets the buffer pool size for operations employing a buffer access strategy."),

The description should mention vacuum, if that's the scope of the GUC's
behavior.

> +#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
> +				# > 0 to specify size

If I'm not wrong, there's still no documentation about "ring buffers" or
postgres' "strategy".  Which seems important to do for this patch, along
with other documentation.

This patch should add support in vacuumdb.c.  And maybe a comment about
adding support there, since it's annoying when it the release notes one
year say "support VACUUM (FOO)" and then one year later say "support
vacuumdb --foo".

-- 
Justin