Re: [HACKERS] Block level parallel vacuum

On Wed, Dec 4, 2019 at 2:01 AM Masahiko Sawada
<masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
>
> On Tue, 3 Dec 2019 at 11:57, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> >
> > Forgot one minor point. Please run pgindent on all the patches.
>
> Got it. I will run pgindent before sending patch from next time.
>

Today, I again read the patch and found a few more minor comments:

1.
void
-LaunchParallelWorkers(ParallelContext *pcxt)
+LaunchParallelWorkers(ParallelContext *pcxt, int nworkers)

I think we should add a comment for this API change which should
indicate why we need to pass nworkers as an additional parameter when
the context itself contains information about the number of workers.

2.
At the beginning of a lazy vacuum (at lazy_scan_heap) we
+ * prepare the parallel context and initialize the DSM segment that contains
+ * shared
information as well as the memory space for storing dead tuples.
+ * When starting either index vacuuming or index cleanup, we launch parallel
+ *
worker processes. Once all indexes are processed the parallel worker
+ * processes exit. And then the leader process re-initializes the parallel
+ *
context so that it can use the same DSM for multiple passses of index
+ * vacuum and for performing index cleanup.

a. /And then the leader/After that, the leader .. This will avoid
using 'and' two times in this sentence.
b. typo, /passses/passes

3.
+ * Macro to check if we are in a parallel lazy vacuum. If true, we are
+ * in the parallel mode and prepared the DSM segment.

How about changing it slightly as /and prepared the DSM segment./ and
the DSM segment is initialized.?

4.
-
/* non-export function prototypes */
static void lazy_scan_heap(Relation onerel, VacuumParams *params,

LVRelStats *vacrelstats, Relation *Irel, int nindexes,
bool aggressive);

Spurious change, please remove. I think this is done by me in one of
the versions.

5.
+ * function we exit from parallel mode. Index bulk-deletion results are
+ * stored in the DSM segment and update index
statistics as a whole after
+ * exited from parallel mode since all writes are not allowed during parallel
+ * mode.

Can we slightly change the above sentence as "Index bulk-deletion
results are stored in the DSM segment and we update index statistics
as a whole after exited from parallel mode since writes are not
allowed during the parallel mode."?

6.
/*
+ * Reset the local value so that we compute cost balance during
+ * parallel index vacuuming.
+
*/

This comment is a bit unclear. How about "Reset the local cost values
for leader backend as we have already accumulated the remaining
balance of heap."?

7.
+ /* Do vacuum or cleanup one index */

How about changing it as: Do vacuum or cleanup of the index?

8.
The copying the result normally
+ * happens only after the first time of index vacuuming.

/The copying the ../The copying of the

9.
+ /*
+ * no longer need the locally allocated result and now
+ * stats[idx] points to the DSM segment.
+
*/

How about changing it as below:
"Now that the stats[idx] points to the DSM segment, we don't need the
locally allocated results."

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com