On Sun, Oct 31, 2021 at 6:21 AM Andres Freund <and...@anarazel.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
