Re: [HACKERS] Block level parallel vacuum

On Wed, 8 Jan 2020 at 22:16, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Jan 4, 2020 at 6:48 PM Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
> >
> > Hi All,
> >
> > In other thread "parallel vacuum options/syntax" [1], Amit Kapila asked opinion about syntax for making normal
vacuumto parallel.  From that thread, I can see that people are in favor of option(b) to implement.  So I tried to
implementoption(b) on the top of v41 patch set and implemented a delta patch. 
> >
>
> I looked at your code and changed it slightly to allow the vacuum to
> be performed in parallel by default.  Apart from that, I have made a
> few other modifications (a) changed the macro SizeOfLVDeadTuples as
> preferred by Tomas [1], (b) updated the documentation, (c) changed a
> few comments.

Thanks.

>
> The first two patches are the same.  I have not posted the patch
> related to the FAST option as I am not sure we have a consensus for
> that and I have also intentionally left DISABLE_LEADER_PARTICIPATION
> related patch to avoid confusion.
>
> What do you think of the attached?  Sawada-san, kindly verify the
> changes and let me know your opinion.

I agreed to not include both the FAST option patch and
DISABLE_LEADER_PARTICIPATION patch at this stage. It's better to focus
on the main part and we can discuss and add them later if want.

I've looked at the latest version patch you shared. Overall it looks
good and works fine. I have a few small comments:

1.
+      refer to <xref linkend="vacuum-phases"/>).  If the
+      <literal>PARALLEL</literal>option or parallel degree

A space is needed between </literal> and 'option'.

2.
+       /*
+        * Variables to control parallel index vacuuming.  We have a bitmap to
+        * indicate which index has stats in shared memory.  The set bit in the
+        * map indicates that the particular index supports a parallel vacuum.
+        */
+       pg_atomic_uint32 idx;           /* counter for vacuuming and clean up */
+       pg_atomic_uint32 nprocessed;    /* # of indexes done during parallel
+
  * execution */
+       uint32          offset;                 /* sizeof header incl. bitmap */
+       bits8           bitmap[FLEXIBLE_ARRAY_MEMBER];  /* bit map of NULLs */
+
+       /* Shared index statistics data follows at end of struct */
+} LVShared;

It seems to me that we no longer use nprocessed at all. So we can remove it.

3.
+ * Compute the number of parallel worker processes to request.  Both index
+ * vacuuming and index cleanup can be executed with parallel workers.  The
+ * relation sizes of table don't affect to the parallel degree for now.

I think the last sentence should be "The relation size of table
doesn't affect to the parallel degree for now".

4.
+       /* cap by max_parallel_maintenance_workers */
+       parallel_workers = Min(parallel_workers,
max_parallel_maintenance_workers);

+       /*
+        * a parallel vacuum must be requested and there must be indexes on the
+        * relation
+        */

+       /* copy the updated statistics */

+       /* parallel vacuum must be active */
+       Assert(VacuumSharedCostBalance);

All comments that the patches newly added except for the above four
places start with a capital letter. Maybe we can change them for
consistency.

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services