Re: parallel vacuum comments

On Sun, Oct 31, 2021 at 6:21 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> Due to bug #17245: [1] I spent a considerably amount of time looking at vacuum
> related code. And I found a few things that I think could stand improvement:
>
> - There's pretty much no tests. This is way way too complicated feature for
> that. If there had been tests for the obvious edge case of some indexes
> being too small to be handled in parallel, but others needing parallelism,
> the mistake leading to #17245 would have been caught during development.

Yes. We should have tests at least for such cases.

>
>
> - There should be error check verifying that all indexes have actually been
> vacuumed. It's way too easy to have bugs leading to index vacuuming being
> skipped.

Agreed.

>
>
> - The state machine is complicated. It's very unobvious that an index needs to
> be processed serially by the leader if shared_indstats == NULL.

I think we can consolidate the logic that decides who (a worker or the
leader) processes the index in one function.

>
>
> - I'm very confused by the existance of LVShared->bitmap. Why is it worth
> saving 7 bits per index for something like this (compared to a simple
> array of bools)? Nor does the naming explain what it's for.
>
> The presence of the bitmap requires stuff like SizeOfLVShared(), which
> accounts for some of the bitmap size, but not all?

Yes, it's better to account for the size of all bitmaps.

>
> But even though we have this space optimized bitmap thing, we actually need
> more memory allocated for each index, making this whole thing pointless.

Right. But is better to change to use booleans?

> - Imo it's pretty confusing to have functions like
> lazy_parallel_vacuum_indexes() (in 13, renamed in 14) that "Perform index
> vacuum or index cleanup with parallel workers.", based on
> lps->lvshared->for_cleanup.

Okay. We need to set lps->lvshared->for_cleanup to tell worker do
either index vacuum or index cleanup. So it might be better to pass
for_cleanup flag down to the functions in addition to setting
lps->lvshared->for_cleanup.

>
>
> - I don't like some of the new names introduced in 14. E.g.
> "do_parallel_processing" is way too generic.

I listed the function names that probably needs to be renamed from
that perspecti:

* do_parallel_processing
* do_serial_processing_for_unsafe_indexes
* parallel_process_one_index

Is there any other function that should be renamed?

> - On a higher level, a lot of this actually doesn't seem to belong into
> vacuumlazy.c, but should be somewhere more generic. Pretty much none of this
> code is heap specific. And vacuumlazy.c is large enough without the parallel
> code.

I don't come with an idea to make them more generic. Could you
elaborate on that?

I've started to write a patch for these comments.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/e