Hello. # Is this still living? I changed the status to "needs review"
At Sat, 6 Apr 2019 06:47:32 +0900, Masahiko Sawada <sawada.m...@gmail.com> wrote in <CAD21AoAuD3txrxucnVtM6NGo=jgsjs3vdkoczn0jgz_egc_...@mail.gmail.com> > > Indeed. How about the following description? > > > > Attached the updated version patches. Thanks. heapam.h is including access/parallel.h but the file doesn't use parallel.h stuff and storage/shm_toc.h and storage/dsm.h are enough. + * DSM keys for parallel lazy vacuum. Since we don't need to worry about DSM + * keys conflicting with plan_node_id we can use small integers. Yeah, this is right, but "plan_node_id" seems abrupt there. Please prepend "differently from parallel execution code" or .. I think no excuse is needed to use that numbers. The executor code is already making an excuse for the large numbers as unusual instead. + * Macro to check if we in a parallel lazy vacuum. If true, we're in parallel + * mode and prepared the DSM segments. + */ +#define IsInParallelVacuum(lps) (((LVParallelState *) (lps)) != NULL) we *are* in? The name "IsInParallleVacuum()" looks (to me) like suggesting "this process is a parallel vacuum worker". How about ParallelVacuumIsActive? +typedef struct LVIndStats +typedef struct LVDeadTuples +typedef struct LVShared +typedef struct LVParallelState The names are confusing, and the name LVShared is too generic. Shared-only structs are better to be marked in the name. That is, maybe it would be better that LVIndStats were LVSharedIndStats and LVShared were LVSharedRelStats. It might be better that LVIndStats were moved out from LVShared, but I'm not confident. +static void +lazy_parallel_vacuum_or_cleanup_indexes(LVRelStats *vacrelstats, Relation *Irel ... + lazy_begin_parallel_index_vacuum(lps, vacrelstats, for_cleanup); ... + do_parallel_vacuum_or_cleanup_indexes(Irel, nindexes, stats, + lps->lvshared, vacrelstats->dead_tuples); ... + lazy_end_parallel_index_vacuum(lps, !for_cleanup); The function takes the parameter for_cleanup, but the flag is used by the three subfunctions in utterly ununified way. It seems to me useless to store for_cleanup in lvshared and lazy_end is rather confusing. There's no explanation why "reinitialization" == "!for_cleanup". In the first place, lazy_begin_parallel_index_vacuum and lazy_end_parallel_index_vacuum are called only from the function and rather short so it doesn't seem reasonable that the are independend functions. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
