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