Re: [HACKERS] Block level parallel vacuum

Hello.

# Is this still living? I changed the status to "needs review"

At Sat, 6 Apr 2019 06:47:32 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoAuD3txrxucnVtM6NGo=JGSjs3VDkoCzN0jGz_egc_82g(at)mail(dot)gmail(dot)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