Re: [HACKERS] Block level parallel vacuum

On Thu, Dec 5, 2019 at 12:22 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> I think it could be required for the cases where the AM doesn't have a
> way (or it is difficult to come up with a way) to communicate the
> stats allocated by the first ambulkdelete call to the subsequent ones
> until amvacuumcleanup.  Currently, we have such a case for the Gist
> index, see email thread [1]. Though we have come up with a way to
> avoid that for Gist indexes, I am not sure if we can assume that it is
> the case for any possible index AM especially when there is a
> provision that indexAM can have additional stats information.  In the
> worst case, if we have to modify some existing index AM like we did
> for the Gist index, we need such a provision so that it is possible.
> In the ideal case, the index AM should provide a way to copy such
> stats, but we can't assume that, so we come up with this option.
>
> We have used this for dummy_index_am which also provides a way to test it.

I think it might be a good idea to change what we expect index AMs to
do rather than trying to make anything that they happen to be doing
right now work, no matter how crazy. In particular, suppose we say
that you CAN'T add data on to the end of IndexBulkDeleteResult any
more, and that instead the extra data is passed through a separate
parameter. And then you add an estimate method that gives the size of
the space provided by that parameter (and if the estimate method isn't
defined then the extra parameter is passed as NULL) and document that
the data stored there might get flat-copied. Now, you've taken the
onus off of parallel vacuum to cope with any crazy thing a
hypothetical AM might be doing, and instead you've defined the
behavior of that hypothetical AM as wrong. If somebody really needs
that, it's now their job to modify the index AM machinery further
instead of your job to somehow cope.

> Here, we have a need to reduce the number of workers.  Index Vacuum
> has two different phases (index vacuum and index cleanup) which uses
> the same parallel-context/DSM but both could have different
> requirements for workers.  The second phase (cleanup) would normally
> need fewer workers as if the work is done in the first phase, second
> wouldn't need it, but we have exceptions like gin indexes where we
> need it for the second phase as well because it takes the pass
> over-index again even if we have cleaned the index in the first phase.
> Now, consider the case where we have 3 btree indexes and 2 gin
> indexes, we would need 5 workers for index vacuum phase and 2 workers
> for index cleanup phase.  There are other cases too.
>
> We also considered to have a separate DSM for each phase, but that
> appeared to have overhead without much benefit.

How about adding an additional argument to ReinitializeParallelDSM()
that allows the number of workers to be reduced? That seems like it
would be less confusing than what you have now, and would involve
modify code in a lot fewer places.

> > Is there any legitimate use case for parallel vacuum in combination
> > with vacuum cost delay?
> >
>
> Yeah, we also initially thought that it is not legitimate to use a
> parallel vacuum with a cost delay.  But to get a wider view, we
> started a separate thread [2] and there we reach to the conclusion
> that we need a solution for throttling [3].

OK, thanks for the pointer. This doesn't address the other part of my
complaint, though, which is that the whole discussion between you and
Dilip and Sawada-san presumes that you want the delays ought to be
scattered across the workers roughly in proportion to their share of
the I/O, and it seems NOT AT ALL clear that this is actually a
desirable property. You're all assuming that, but none of you has
justified it, and I think the opposite might be true in some cases.
You're adding extra complexity for something that isn't a clear
improvement.

> Your understanding is correct.  How about if we modify it to something
> like: "Note that parallel workers are alive only during index vacuum
> or index cleanup but the leader process neither exits from the
> parallel mode nor destroys the parallel context until the entire
> parallel operation is finished." OR something like "The leader backend
> holds the parallel context till the index vacuum and cleanup is
> finished.  Both index vacuum and cleanup separately perform the work
> with parallel workers."

How about if you just delete it? You don't need a comment explaining
that this caller of CreateParallelContext() does something which
*every* caller of CreateParallelContext() must do. If you didn't do
that, you'd fail assertions and everything would break, so *of course*
you are doing it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company