Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode - Mailing list pgsql-hackers

Thanks for the review!

On Sat, Mar 11, 2023 at 2:16 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> 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.

Agreed. It definitely needs a comment like this. I will update in the
next version along with addressing your other feedback (after sorting
out some of the other points in this mail on which I still have
questions).

> > 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

Yes, there doesn't seem to be a great amount of consistency around
this... See pgstat.c read_chunk_s and bufmgr.c BufHdrGetBlock and
friends. Though there are probably more capitalized than not. Since it
does a bit of math and returns a value, I wanted to convey that it was
more like a function. Also, since the name was long, I thought all-caps
would be hard to read. However, if you or others feel strongly, I am
attached neither to the capitalization nor to the name at all (what do
you think of the name?).

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

Ah yes, this is a good idea -- I always miss at least one set of
parentheses when writing a macro. In this case, I didn't think of it
since the current caller couldn't pass an expression.

> > @@ -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.

You mean instead of having anyone call GetAccessStrategyWithSize()?
We would need to change the signature of GetAccessStrategy() then -- and
there are quite a few callers.

>
> > +             {"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.

Good catch. Will update in next version.

> > +#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.

Yes, it is. I have been thinking about where in the docs to add it (the
docs about buffer access strategies). Any ideas?

> This patch should add support in vacuumdb.c.

Oh, I had totally forgotten about vacuumdb.

> 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".

I'm not sure I totally follow. Do you mean to add a comment to
ExecVacuum() saying to add support to vacuumdb when adding a new option
to vacuum?

In the past, did people forget to add support to vacuumdb for new vacuum
options or did they forget to document that they did that or did they
forgot to include that they did that in the release notes?

- Melanie