On Thu, Dec 9, 2021 at 7:05 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Dec 6, 2021 at 10:17 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Fri, Dec 3, 2021 at 6:06 PM Masahiko Sawada <sawada.m...@gmail.com> > > wrote: > > > > > > On Fri, Dec 3, 2021 at 6:03 PM Amit Kapila <amit.kapil...@gmail.com> > > > wrote: > > > > > > > > On Thu, Dec 2, 2021 at 6:01 PM Masahiko Sawada <sawada.m...@gmail.com> > > > > wrote: > > > > > > > > > > I've attached updated patches. > > > > > > > > > > > > > I have a few comments on v4-0001. > > > > > > Thank you for the comments! > > > > > > > 1. > > > > In parallel_vacuum_process_all_indexes(), we can combine the two > > > > checks for vacuum/cleanup at the beginning of the function > > > > > > Agreed. > > > > > > > and I think > > > > it is better to keep the variable name as bulkdel or cleanup instead > > > > of vacuum as that is more specific and clear. > > > > > > I was thinking to use the terms "bulkdel" and "cleanup" instead of > > > "vacuum" and "cleanup" for the same reason. That way, probably we can > > > use “bulkdel" and “cleanup" when doing index bulk-deletion (i.g., > > > calling to ambulkdelete) and index cleanup (calling to > > > amvacuumcleanup), respectively, and use "vacuum" when processing an > > > index, i.g., doing either bulk-delete or cleanup, instead of using > > > just "processing" . But we already use “vacuum” and “cleanup” in many > > > places, e.g., lazy_vacuum_index() and lazy_cleanup_index(). If we want > > > to use “bulkdel” instead of “vacuum”, I think it would be better to > > > change the terminology in vacuumlazy.c thoroughly, probably in a > > > separate patch. > > > > > > > Okay. > > > > > > 2. The patch seems to be calling parallel_vacuum_should_skip_index > > > > thrice even before starting parallel vacuum. It has a call to find the > > > > number of blocks which has to be performed for each index. I > > > > understand it might not be too costly to call this but it seems better > > > > to remember this info like we are doing in the current code. > > > > > > Yes, we can bring will_vacuum_parallel array back to the code. That > > > way, we can remove the call to parallel_vacuum_should_skip_index() in > > > parallel_vacuum_begin(). > > > > > > > We can > > > > probably set parallel_workers_can_process in parallel_vacuum_begin and > > > > then again update in parallel_vacuum_process_all_indexes. Won't doing > > > > something like that be better? > > > > > > parallel_workers_can_process can vary depending on bulk-deletion, the > > > first time cleanup, or the second time (or more) cleanup. What can we > > > set parallel_workers_can_process based on in parallel_vacuum_begin()? > > > > > > > I was thinking to set the results of will_vacuum_parallel in > > parallel_vacuum_begin(). > > > > This point doesn't seem to be addressed in the latest version (v6). Is > there a reason for not doing it? If we do this, then we don't need to > call parallel_vacuum_should_skip_index() from > parallel_vacuum_index_is_parallel_safe().
Probably I had misunderstood your point. I'll fix it in the next version patch and send it soon. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
