On Mon, Dec 20, 2021 at 6:29 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Mon, Dec 20, 2021 at 1:08 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > > > > > 2. What is the reason for not moving > > > > lazy_vacuum_one_index/lazy_cleanup_one_index to vacuum.c so that they > > > > can be called from vacuumlazy.c and vacuumparallel.c? Without this > > > > refactoring patch, I think both leader and workers set the same error > > > > context phase (VACUUM_ERRCB_PHASE_VACUUM_INDEX) during index > > > > vacuuming? Is it because you want a separate context phase for a > > > > parallel vacuum? > > > > > > Since the phases defined as VacErrPhase like > > > VACUUM_ERRCB_PHASE_SCAN_HEAP and VACUUM_ERRCB_PHASE_VACUUM_HEAP etc. > > > and error callback function, vacuum_error_callback(), are specific to > > > heap, I thought it'd not be a good idea to move > > > lazy_vacuum/cleanup_one_index() so that both vacuumlazy.c and > > > vacuumparallel.c can use the phases and error callback function. > > > > > > > How about exposing it via heapam.h? We have already exposed a few > > things via heapam.h (see /* in heap/vacuumlazy.c */). In the current > > proposal, we need to have separate callbacks and phases for index > > vacuuming so that it can be used by both vacuumlazy.c and > > vacuumparallel.c which might not be a good idea. > > Yeah, but if we expose VacErrPhase and vacuum_error_callback(), we > need to also expose LVRelState and vacuumparallel.c also uses it, > which seems not a good idea. So we will need to introduce a new struct > dedicated to the error callback function. Is that right? >
Right, but that also doesn't sound good to me. I think it is better to keep a separate error context for parallel vacuum workers as you have in the patch. However, let's add some comments to indicate that if there is a change in one of the context functions, the other should be changed. BTW, if we go with that then we should set the correct phase for workers as well? -- With Regards, Amit Kapila.
