Re: [HACKERS] Block level parallel vacuum
От | Masahiko Sawada |
---|---|
Тема | Re: [HACKERS] Block level parallel vacuum |
Дата | |
Msg-id | CAD21AoCX3fmXJrPUWd9H2_iMMtdh32XN7qRx1udWD7aeD0=G_Q@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [HACKERS] Block level parallel vacuum (Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>) |
Ответы |
Re: [HACKERS] Block level parallel vacuum
(Masahiko Sawada <sawada.mshk@gmail.com>)
|
Список | pgsql-hackers |
On Mon, Apr 8, 2019 at 7:25 PM Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > > Hello. > > # Is this still living? I changed the status to "needs review" > > At Sat, 6 Apr 2019 06:47:32 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoAuD3txrxucnVtM6NGo=JGSjs3VDkoCzN0jGz_egc_82g@mail.gmail.com> > > > Indeed. How about the following description? > > > > > > > Attached the updated version patches. > > Thanks. > Thank you for reviewing the patch! > 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. Fixed. > > + * 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. Fixed. > > + * 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? Fixed. > > The name "IsInParallleVacuum()" looks (to me) like suggesting > "this process is a parallel vacuum worker". How about > ParallelVacuumIsActive? Fixed. > > > +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. Hmm, LVShared actually stores also various things that are not relevant with the relation. I'm not sure that's a good idea to rename it to LVSharedRelStats. When we support parallel vacuum for other vacuum steps the adding a struct for storing only relation statistics might work well. > > 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 I think that we need to store for_cleanup or a something telling vacuum workers to do either index vacuuming or index cleanup in lvshared. Or can we use another thing instead? > and lazy_end is > rather confusing. Ah, I used "lazy" as prefix of function in vacuumlazy.c. Fixed. > 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. Okay agreed, fixed. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
В списке pgsql-hackers по дате отправления:
Следующее
От: Amit KapilaДата:
Сообщение: Re: Transaction commits VS Transaction commits (with parallel) VSquery mean time