On Wed, Nov 27, 2019 at 8:14 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Nov 27, 2019 at 12:52 AM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> >
> > I've incorporated the comments I got so far including the above and
> > the memory alignment issue.
> >
>
> Thanks, I will look into the new version.
>
Few comments:
-----------------------
1.
+static void
+vacuum_or_cleanup_indexes_worker(Relation *Irel, int nindexes,
+ IndexBulkDeleteResult **stats,
+ LVShared *lvshared,
+ LVDeadTuples *dead_tuples)
+{
+ /* Increment the active worker count */
+ pg_atomic_add_fetch_u32(VacuumActiveNWorkers, 1);
The above code is wrong because it is possible that this function is
called even when there are no workers in which case
VacuumActiveNWorkers will be NULL.
2.
+ /* Take over the shared balance value to heap scan */
+ VacuumCostBalance = pg_atomic_read_u32(VacuumSharedCostBalance);
We can carry over shared balance only if the same is active.
3.
+ if (Irel[i]->rd_indam->amparallelvacuumoptions ==
+ VACUUM_OPTION_NO_PARALLEL)
+ {
+
/* Set NULL as this index does not support parallel vacuum */
+ lvshared->bitmap[i >> 3] |= 0 << (i & 0x07);
Can we avoid setting this for each index by initializing bitmap as all
NULL's as is done in the attached patch?
4.
+ /*
+ * Variables to control parallel index vacuuming. Index statistics
+ * returned from ambulkdelete and amvacuumcleanup is nullable
variable
+ * length. 'offset' is NULL bitmap. Note that a 0 indicates a null,
+ * while 1 indicates non-null. The index statistics follows
at end of
+ * struct.
+ */
This comment is not clear, so I have re-worded it. See, if the
changed comment makes sense.
I have fixed all the above issues, made a couple of other cosmetic
changes and modified a few comments. See the changes in
v34-0002-delta-amit. I am attaching just the delta patch on top of
v34-0002-Add-parallel-option-to-VACUUM-command.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
