Re: [HACKERS] Block level parallel vacuum
От | Masahiko Sawada |
---|---|
Тема | Re: [HACKERS] Block level parallel vacuum |
Дата | |
Msg-id | CAD21AoD8rVHPYu5iSdAXf3CmrBPakUyquUwzOWtK3K_4t9oZng@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [HACKERS] Block level parallel vacuum (Masahiko Sawada <sawada.mshk@gmail.com>) |
Ответы |
Re: [HACKERS] Block level parallel vacuum
(Sergei Kornilov <sk@zsrv.org>)
Re: [HACKERS] Block level parallel vacuum (Amit Kapila <amit.kapila16@gmail.com>) |
Список | pgsql-hackers |
On Wed, Apr 10, 2019 at 2:19 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > 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. > Since the previous version patch conflicts with current HEAD, I've attached the updated version patches. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Вложения
В списке pgsql-hackers по дате отправления: