Horton

Thread

Commits

  1. Move parallel vacuum code to vacuumparallel.c.

  2. Move index vacuum routines to vacuum.c.

  3. Improve parallel vacuum implementation.

  4. Fix parallel amvacuumcleanup safety bug.

  5. Simplify state managed by VACUUM.

  1. parallel vacuum comments

    Andres Freund <andres@anarazel.de> — 2021-10-30T21:21:01Z

    Hi,

Due to bug #17245: [1] I spent a considerably amount of time looking at vacuum
related code. And I found a few things that I think could stand improvement:

- There's pretty much no tests. This is way way too complicated feature for
  that. If there had been tests for the obvious edge case of some indexes
  being too small to be handled in parallel, but others needing parallelism,
  the mistake leading to #17245 would have been caught during development.


- There should be error check verifying that all indexes have actually been
  vacuumed. It's way too easy to have bugs leading to index vacuuming being
  skipped.


- The state machine is complicated. It's very unobvious that an index needs to
  be processed serially by the leader if shared_indstats == NULL.


- I'm very confused by the existance of LVShared->bitmap. Why is it worth
  saving 7 bits per index for something like this (compared to a simple
  array of bools)? Nor does the naming explain what it's for.

  The presence of the bitmap requires stuff like SizeOfLVShared(), which
  accounts for some of the bitmap size, but not all?

  But even though we have this space optimized bitmap thing, we actually need
  more memory allocated for each index, making this whole thing pointless.


- Imo it's pretty confusing to have functions like
  lazy_parallel_vacuum_indexes() (in 13, renamed in 14) that "Perform index
  vacuum or index cleanup with parallel workers.", based on
  lps->lvshared->for_cleanup.


- I don't like some of the new names introduced in 14. E.g.
  "do_parallel_processing" is way too generic.


- On a higher level, a lot of this actually doesn't seem to belong into
  vacuumlazy.c, but should be somewhere more generic. Pretty much none of this
  code is heap specific. And vacuumlazy.c is large enough without the parallel
  code.


Greetings,

Andres Freund

[1] https://postgr.es/m/17245-ddf06aaf85735f36%40postgresql.org

  2. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-01T01:44:34Z

    On Sun, Oct 31, 2021 at 6:21 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> Due to bug #17245: [1] I spent a considerably amount of time looking at vacuum
> related code. And I found a few things that I think could stand improvement:
>
> - There's pretty much no tests. This is way way too complicated feature for
>   that. If there had been tests for the obvious edge case of some indexes
>   being too small to be handled in parallel, but others needing parallelism,
>   the mistake leading to #17245 would have been caught during development.

Yes. We should have tests at least for such cases.

>
>
> - There should be error check verifying that all indexes have actually been
>   vacuumed. It's way too easy to have bugs leading to index vacuuming being
>   skipped.

Agreed.

>
>
> - The state machine is complicated. It's very unobvious that an index needs to
>   be processed serially by the leader if shared_indstats == NULL.

I think we can consolidate the logic that decides who (a worker or the
leader) processes the index in one function.

>
>
> - I'm very confused by the existance of LVShared->bitmap. Why is it worth
>   saving 7 bits per index for something like this (compared to a simple
>   array of bools)? Nor does the naming explain what it's for.
>
>   The presence of the bitmap requires stuff like SizeOfLVShared(), which
>   accounts for some of the bitmap size, but not all?

Yes, it's better to account for the size of all bitmaps.

>
>   But even though we have this space optimized bitmap thing, we actually need
>   more memory allocated for each index, making this whole thing pointless.

Right. But is better to change to use booleans?

> - Imo it's pretty confusing to have functions like
>   lazy_parallel_vacuum_indexes() (in 13, renamed in 14) that "Perform index
>   vacuum or index cleanup with parallel workers.", based on
>   lps->lvshared->for_cleanup.

Okay. We need to set lps->lvshared->for_cleanup to tell worker do
either index vacuum or index cleanup. So it might be better to pass
for_cleanup flag down to the functions in addition to setting
lps->lvshared->for_cleanup.

>
>
> - I don't like some of the new names introduced in 14. E.g.
>   "do_parallel_processing" is way too generic.

I listed the function names that probably needs to be renamed from
that perspecti:

* do_parallel_processing
* do_serial_processing_for_unsafe_indexes
* parallel_process_one_index

Is there any other function that should be renamed?


> - On a higher level, a lot of this actually doesn't seem to belong into
>   vacuumlazy.c, but should be somewhere more generic. Pretty much none of this
>   code is heap specific.  And vacuumlazy.c is large enough without the parallel
>   code.

I don't come with an idea to make them more generic. Could you
elaborate on that?

I've started to write a patch for these comments.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/e

  3. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-01T12:47:17Z

    On Mon, Nov 1, 2021 at 10:44 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sun, Oct 31, 2021 at 6:21 AM Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > Due to bug #17245: [1] I spent a considerably amount of time looking at vacuum
> > related code. And I found a few things that I think could stand improvement:
> >
> > - There's pretty much no tests. This is way way too complicated feature for
> >   that. If there had been tests for the obvious edge case of some indexes
> >   being too small to be handled in parallel, but others needing parallelism,
> >   the mistake leading to #17245 would have been caught during development.
>
> Yes. We should have tests at least for such cases.

For discussion, I've written a patch only for adding some tests to
parallel vacuum. The test includes the reported case where small
indexes are not processed by the leader process as well as cases where
different kinds of indexes (i.g., different amparallelvacuumoptions)
are vacuumed or cleaned up.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  4. Re: parallel vacuum comments

    Peter Geoghegan <pg@bowt.ie> — 2021-11-01T20:57:01Z

    On Mon, Nov 1, 2021 at 5:47 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> For discussion, I've written a patch only for adding some tests to
> parallel vacuum. The test includes the reported case where small
> indexes are not processed by the leader process as well as cases where
> different kinds of indexes (i.g., different amparallelvacuumoptions)
> are vacuumed or cleaned up.

I started looking at this because I want to commit something like it
alongside a fix to bug #17245.

While I tend to favor relatively heavy assertions (e.g., the
heap_page_is_all_visible() related asserts I added to
lazy_scan_prune()), the idea of having a whole shared memory area just
for assertions seems a bit too much, even to me. I tried to simplify
it by just adding a new field to LVSharedIndStats, which seemed more
natural. It took me at least 15 minutes before I realized that I was
actually repeating exactly the same mistake that led to bug #17245 in
the first place. I somehow forgot that
parallel_stats_for_idx()/IndStatsIsNull() will return NULL for any
index that has already been deemed too small to be worth processing in
parallel. Even after all that drama!

Rather than inventing PARALLEL_VACUUM_KEY_INDVAC_CHECK (just for
assert-enabled builds), we should invent PARALLEL_VACUUM_STATS -- a
dedicated shmem area for the array of LVSharedIndStats (no more
storing LVSharedIndStats entries at the end of the LVShared space in
an ad-hoc, type unsafe way). There should be one array element for
each and every index -- even those indexes where parallel index
vacuuming is unsafe or not worthwhile (unsure if avoiding parallel
processing for "not worthwhile" indexes actually makes sense, BTW). We
can then get rid of the bitmap/IndStatsIsNull() stuff entirely. We'd
also add new per-index state fields to LVSharedIndStats itself. We
could directly record the status of each index (e.g., parallel unsafe,
amvacuumcleanup processing done, ambulkdelete processing done)
explicitly. All code could safely subscript the LVSharedIndStats array
directly, using idx style integers. That seems far more robust and
consistent.

I think that this PARALLEL_VACUUM_STATS refactoring is actually the
simplest way to comprehensively test parallel VACUUM. I will still
need to add tests for my fix to bug #17245, but they won't be truly
general tests. I'll have to make sure that one of the assertions in
nbtdedup.c fails when the tests are run without the fix in place, or
something like that.

-- 
Peter Geoghegan

  5. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-02T05:46:23Z

    On Tue, Nov 2, 2021 at 5:57 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Mon, Nov 1, 2021 at 5:47 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > For discussion, I've written a patch only for adding some tests to
> > parallel vacuum. The test includes the reported case where small
> > indexes are not processed by the leader process as well as cases where
> > different kinds of indexes (i.g., different amparallelvacuumoptions)
> > are vacuumed or cleaned up.
>
> I started looking at this because I want to commit something like it
> alongside a fix to bug #17245.
>
> While I tend to favor relatively heavy assertions (e.g., the
> heap_page_is_all_visible() related asserts I added to
> lazy_scan_prune()), the idea of having a whole shared memory area just
> for assertions seems a bit too much, even to me. I tried to simplify
> it by just adding a new field to LVSharedIndStats, which seemed more
> natural. It took me at least 15 minutes before I realized that I was
> actually repeating exactly the same mistake that led to bug #17245 in
> the first place. I somehow forgot that
> parallel_stats_for_idx()/IndStatsIsNull() will return NULL for any
> index that has already been deemed too small to be worth processing in
> parallel. Even after all that drama!

The idea of that patch was for back branches in order to not affect
non-enable-cassert builds. That said, I agree that it's an overkill
solution.

> Rather than inventing PARALLEL_VACUUM_KEY_INDVAC_CHECK (just for
> assert-enabled builds), we should invent PARALLEL_VACUUM_STATS -- a
> dedicated shmem area for the array of LVSharedIndStats (no more
> storing LVSharedIndStats entries at the end of the LVShared space in
> an ad-hoc, type unsafe way). There should be one array element for
> each and every index -- even those indexes where parallel index
> vacuuming is unsafe or not worthwhile (unsure if avoiding parallel
> processing for "not worthwhile" indexes actually makes sense, BTW). We
> can then get rid of the bitmap/IndStatsIsNull() stuff entirely. We'd
> also add new per-index state fields to LVSharedIndStats itself. We
> could directly record the status of each index (e.g., parallel unsafe,
> amvacuumcleanup processing done, ambulkdelete processing done)
> explicitly. All code could safely subscript the LVSharedIndStats array
> directly, using idx style integers. That seems far more robust and
> consistent.

Sounds good.

During the development, I wrote the patch while considering using
fewer shared memory but it seems that it brought complexity (and
therefore the bug). It would not be harmful even if we allocate index
statistics on DSM for unsafe indexes and “not worthwhile" indexes in
practice. Rather, tracking bulkdelete and vacuumcleanup completion
might enable us to improve the vacuum progress reporting so that the
progress stats view shows how many indexes have been vacuumed (or
cleaned up).

Anyway, I'll write a patch accordingly.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  6. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-02T09:27:28Z

    On Mon, Nov 1, 2021 at 7:15 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sun, Oct 31, 2021 at 6:21 AM Andres Freund <andres@anarazel.de> wrote:
>
> > - Imo it's pretty confusing to have functions like
> >   lazy_parallel_vacuum_indexes() (in 13, renamed in 14) that "Perform index
> >   vacuum or index cleanup with parallel workers.", based on
> >   lps->lvshared->for_cleanup.
>
> Okay. We need to set lps->lvshared->for_cleanup to tell worker do
> either index vacuum or index cleanup. So it might be better to pass
> for_cleanup flag down to the functions in addition to setting
> lps->lvshared->for_cleanup.
>

But, we need this information in the parallel worker as well to know
whether to perform index vacuum or clean up, so I guess we need this
information in shared memory, no?

> >
> >
> > - I don't like some of the new names introduced in 14. E.g.
> >   "do_parallel_processing" is way too generic.
>
> I listed the function names that probably needs to be renamed from
> that perspecti:
>
> * do_parallel_processing
> * do_serial_processing_for_unsafe_indexes
> * parallel_process_one_index
>
> Is there any other function that should be renamed?
>
>
> > - On a higher level, a lot of this actually doesn't seem to belong into
> >   vacuumlazy.c, but should be somewhere more generic. Pretty much none of this
> >   code is heap specific.  And vacuumlazy.c is large enough without the parallel
> >   code.
>
> I don't come with an idea to make them more generic. Could you
> elaborate on that?
>

Can we think of moving parallelism-related code to a different file
(say vacuumparallel.c)? At least that will reduce the footprint of
vacuumlazy.c.

-- 
With Regards,
Amit Kapila.

  7. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-03T02:35:04Z

    On Tue, Nov 2, 2021 at 2:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Anyway, I'll write a patch accordingly.

While writing a patch for these comments, I found another bug in
parallel_processing_is_safe():

/*
 * Returns false, if the given index can't participate in parallel index
 * vacuum or parallel index cleanup
 */
static bool
parallel_processing_is_safe(Relation indrel, LVShared *lvshared)
{
    uint8       vacoptions = indrel->rd_indam->amparallelvacuumoptions;

    /* first_time must be true only if for_cleanup is true */
    Assert(lvshared->for_cleanup || !lvshared->first_time);

    if (lvshared->for_cleanup)
    {
        /* Skip, if the index does not support parallel cleanup */
        if (((vacoptions & VACUUM_OPTION_PARALLEL_CLEANUP) == 0) &&
            ((vacoptions & VACUUM_OPTION_PARALLEL_COND_CLEANUP) == 0))
            return true;

It returns true in the above condition but it should return false
since the index doesn't support parallel index cleanup at all. It
seems that this bug was introduced by commit b4af70cb21 (therefore
exists only in PG14) which flipped the return values of this function
but missed one place. The index AMs that don't support parallel index
cleanup at all are affected by this bug. Among the supported index AM
in the core, hash indexes are affected but since they just return the
number of blocks during vacuumcleanup it would not become a serious
consequence.

I've attached a patch to fix it.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  8. Re: parallel vacuum comments

    Peter Geoghegan <pg@bowt.ie> — 2021-11-03T02:53:09Z

    On Tue, Nov 2, 2021 at 7:35 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> It returns true in the above condition but it should return false
> since the index doesn't support parallel index cleanup at all. It
> seems that this bug was introduced by commit b4af70cb21 (therefore
> exists only in PG14) which flipped the return values of this function
> but missed one place. The index AMs that don't support parallel index
> cleanup at all are affected by this bug. Among the supported index AM
> in the core, hash indexes are affected but since they just return the
> number of blocks during vacuumcleanup it would not become a serious
> consequence.
>
> I've attached a patch to fix it.

I pushed your fix just now.

Thanks
-- 
Peter Geoghegan

  9. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-03T02:54:20Z

    On Wed, Nov 3, 2021 at 11:53 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Tue, Nov 2, 2021 at 7:35 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > It returns true in the above condition but it should return false
> > since the index doesn't support parallel index cleanup at all. It
> > seems that this bug was introduced by commit b4af70cb21 (therefore
> > exists only in PG14) which flipped the return values of this function
> > but missed one place. The index AMs that don't support parallel index
> > cleanup at all are affected by this bug. Among the supported index AM
> > in the core, hash indexes are affected but since they just return the
> > number of blocks during vacuumcleanup it would not become a serious
> > consequence.
> >
> > I've attached a patch to fix it.
>
> I pushed your fix just now.

Thanks!

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  10. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-03T04:08:06Z

    On Tue, Nov 2, 2021 at 11:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Nov 2, 2021 at 5:57 AM Peter Geoghegan <pg@bowt.ie> wrote:
> >
>
> > Rather than inventing PARALLEL_VACUUM_KEY_INDVAC_CHECK (just for
> > assert-enabled builds), we should invent PARALLEL_VACUUM_STATS -- a
> > dedicated shmem area for the array of LVSharedIndStats (no more
> > storing LVSharedIndStats entries at the end of the LVShared space in
> > an ad-hoc, type unsafe way). There should be one array element for
> > each and every index -- even those indexes where parallel index
> > vacuuming is unsafe or not worthwhile (unsure if avoiding parallel
> > processing for "not worthwhile" indexes actually makes sense, BTW). We
> > can then get rid of the bitmap/IndStatsIsNull() stuff entirely. We'd
> > also add new per-index state fields to LVSharedIndStats itself. We
> > could directly record the status of each index (e.g., parallel unsafe,
> > amvacuumcleanup processing done, ambulkdelete processing done)
> > explicitly. All code could safely subscript the LVSharedIndStats array
> > directly, using idx style integers. That seems far more robust and
> > consistent.
>
> Sounds good.
>
> During the development, I wrote the patch while considering using
> fewer shared memory but it seems that it brought complexity (and
> therefore the bug). It would not be harmful even if we allocate index
> statistics on DSM for unsafe indexes and “not worthwhile" indexes in
> practice.
>

If we want to allocate index stats for all indexes in DSM then why not
consider it on the lines of buf/wal_usage means tack those via
LVParallelState? And probably replace bitmap with an array of bools
that indicates which indexes can be skipped by the parallel worker.

-- 
With Regards,
Amit Kapila.

  11. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-04T05:24:46Z

    On Wed, Nov 3, 2021 at 1:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Nov 2, 2021 at 11:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Nov 2, 2021 at 5:57 AM Peter Geoghegan <pg@bowt.ie> wrote:
> > >
> >
> > > Rather than inventing PARALLEL_VACUUM_KEY_INDVAC_CHECK (just for
> > > assert-enabled builds), we should invent PARALLEL_VACUUM_STATS -- a
> > > dedicated shmem area for the array of LVSharedIndStats (no more
> > > storing LVSharedIndStats entries at the end of the LVShared space in
> > > an ad-hoc, type unsafe way). There should be one array element for
> > > each and every index -- even those indexes where parallel index
> > > vacuuming is unsafe or not worthwhile (unsure if avoiding parallel
> > > processing for "not worthwhile" indexes actually makes sense, BTW). We
> > > can then get rid of the bitmap/IndStatsIsNull() stuff entirely. We'd
> > > also add new per-index state fields to LVSharedIndStats itself. We
> > > could directly record the status of each index (e.g., parallel unsafe,
> > > amvacuumcleanup processing done, ambulkdelete processing done)
> > > explicitly. All code could safely subscript the LVSharedIndStats array
> > > directly, using idx style integers. That seems far more robust and
> > > consistent.
> >
> > Sounds good.
> >
> > During the development, I wrote the patch while considering using
> > fewer shared memory but it seems that it brought complexity (and
> > therefore the bug). It would not be harmful even if we allocate index
> > statistics on DSM for unsafe indexes and “not worthwhile" indexes in
> > practice.
> >
>
> If we want to allocate index stats for all indexes in DSM then why not
> consider it on the lines of buf/wal_usage means tack those via
> LVParallelState? And probably replace bitmap with an array of bools
> that indicates which indexes can be skipped by the parallel worker.
>

I've attached a draft patch. The patch incorporated all comments from
Andres except for the last comment that moves parallel related code to
another file. I'd like to discuss how we split vacuumlazy.c.

Regarding tests, I’d like to add tests to check if a vacuum with
multiple index scans (i.g., due to small maintenance_work_mem) works
fine. But a problem is that we need at least about 200,000 garbage
tuples to perform index scan twice even with the minimum
maintenance_work_mem. Which takes a time to load tuples.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  12. Re: parallel vacuum comments

    Andres Freund <andres@anarazel.de> — 2021-11-04T19:00:03Z

    Hi,

On 2021-11-01 10:44:34 +0900, Masahiko Sawada wrote:
> On Sun, Oct 31, 2021 at 6:21 AM Andres Freund <andres@anarazel.de> wrote:
> >   But even though we have this space optimized bitmap thing, we actually need
> >   more memory allocated for each index, making this whole thing pointless.
> 
> Right. But is better to change to use booleans?

It seems very clearly better to me. We shouldn't use complicated stuff like

#define SizeOfLVShared (offsetof(LVShared, bitmap) + sizeof(bits8))
#define GetSharedIndStats(s) \
	((LVSharedIndStats *)((char *)(s) + ((LVShared *)(s))->offset))
#define IndStatsIsNull(s, i) \
	(!(((LVShared *)(s))->bitmap[(i) >> 3] & (1 << ((i) & 0x07))))

when there's reason / benefit.


> > - Imo it's pretty confusing to have functions like
> >   lazy_parallel_vacuum_indexes() (in 13, renamed in 14) that "Perform index
> >   vacuum or index cleanup with parallel workers.", based on
> >   lps->lvshared->for_cleanup.
> 
> Okay. We need to set lps->lvshared->for_cleanup to tell worker do
> either index vacuum or index cleanup. So it might be better to pass
> for_cleanup flag down to the functions in addition to setting
> lps->lvshared->for_cleanup.

Yup.


> > - I don't like some of the new names introduced in 14. E.g.
> >   "do_parallel_processing" is way too generic.
> 
> I listed the function names that probably needs to be renamed from
> that perspecti:
> 
> * do_parallel_processing
> * do_serial_processing_for_unsafe_indexes
> * parallel_process_one_index
> 
> Is there any other function that should be renamed?

parallel_processing_is_safe().

I don't like that there's three different naming patterns for parallel
things. There's do_parallel_*, there's parallel_, and there's
(begin|end|compute)_parallel_*.


> > - On a higher level, a lot of this actually doesn't seem to belong into
> >   vacuumlazy.c, but should be somewhere more generic. Pretty much none of this
> >   code is heap specific.  And vacuumlazy.c is large enough without the parallel
> >   code.
> 
> I don't come with an idea to make them more generic. Could you
> elaborate on that?

To me the the job that the parallel vacuum stuff does isn't really specific to
heap. Any table AM supporting indexes is going to need to do something pretty
much like it (it's calling indexam stuff). Most of the stuff in vacuumlazy.c
is very heap specific - you're not going to be able to reuse lazy_scan_heap()
or such. Before the parallel vacuum stuff, the index specific code in
vacuumlazy.c was fairly limited - but now it's a nontrivial amount of code.

Based on a quick look
  parallel_vacuum_main(), parallel_processing_is_safe(),
  parallel_stats_for_idx(), end_parallel_vacuum(), begin_parallel_vacuum(),
  compute_parallel_vacuum_workers(), parallel_process_one_index(),
  do_serial_processing_for_unsafe_indexes(), do_parallel_processing(),
  do_parallel_vacuum_or_cleanup(), do_parallel_lazy_cleanup_all_indexes(),
  do_parallel_lazy_vacuum_all_indexes(),

don't really belong in vacuumlazy.c. but should be in vacuum.c or a new
file. Of course that requires a bit of work, because of the heavy reliance on
LVRelState, but afaict there's not really an intrinsic need to use that.

Greetings,

Andres Freund

  13. Re: parallel vacuum comments

    Peter Geoghegan <pg@bowt.ie> — 2021-11-04T19:42:37Z

    On Wed, Nov 3, 2021 at 10:25 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've attached a draft patch. The patch incorporated all comments from
> Andres except for the last comment that moves parallel related code to
> another file. I'd like to discuss how we split vacuumlazy.c.

This looks great!

I wonder if this is okay, though:

>     /* Process the indexes that can be processed by only leader process */
> -   do_serial_processing_for_unsafe_indexes(vacrel, lps->lvshared);
> +   lazy_serial_process_indexes(vacrel);
>
>     /*
> -    * Join as a parallel worker.  The leader process alone processes all the
> -    * indexes in the case where no workers are launched.
> +    * Join as a parallel worker.  The leader process alone processes all
> +    * parallel-safe indexes in the case where no workers are launched.
>      */
> -   do_parallel_processing(vacrel, lps->lvshared);
> +   lazy_parallel_process_indexes(vacrel, lps->lvshared, vacrel->lps->lvsharedindstats);
>
>     /*
>      * Next, accumulate buffer and WAL usage.  (This must wait for the workers
> @@ -2786,6 +2734,18 @@ do_parallel_vacuum_or_cleanup(LVRelState *vacrel, int nworkers)
>             InstrAccumParallelQuery(&lps->buffer_usage[i], &lps->wal_usage[i]);
>     }

Since "The leader process alone processes all parallel-safe indexes in
the case where no workers are launched" (no change there), I wonder:
how does the leader *know* that it's the leader (and so can always
process any indexes) inside its call to
lazy_parallel_process_indexes()? Or, does the leader actually process
all indexes inside lazy_serial_process_indexes() in this edge case?
(The edge case where a parallel VACUUM has no workers at all, because
they couldn't be launched by the core parallel infrastructure.)

One small thing: the new "LVSharedIndStats.parallel_safe" field seems
to be slightly misnamed. Isn't it more like
"LVSharedIndStats.parallel_workers_can_process"? The index might
actually be parallel safe *in principle*, while nevertheless being
deliberately skipped over by workers due to a deliberate up-front
choice made earlier, in compute_parallel_vacuum_workers(). Most
obvious example of this is the choice to not use parallelism for a
smaller index (say a partial index whose size is <
min_parallel_index_scan_size).

Another small thing, which is closely related to the last one:

>  typedef struct LVSharedIndStats
>  {
> -   bool        updated;        /* are the stats updated? */
> +   LVIndVacStatus status;
> +
> +   /*
> +    * True if both leader and worker can process the index, otherwise only
> +    * leader can process it.
> +    */
> +   bool    parallel_safe;
> +
> +   bool    istat_updated;      /* are the stats updated? */
>     IndexBulkDeleteResult istat;
>  } LVSharedIndStats;

It would be nice to point out that the new
"LVSharedIndStats.parallel_safe" field (or whatever we end up calling
it) had comments that pointed out that it isn't a fixed thing for the
entire VACUUM operation -- it's only fixed for an individual call to
parallel_lazy_vacuum_or_cleanup_all_indexes() (i.e., it's only fixed
for the "ambulkdelete portion" or the "amvacuumcleanup portion" of the
entire VACUUM).

Alternatively, you could just have two booleans, I think. You know,
one for the "ambulkdelete portion", another for the "amvacuumcleanup
portion". As I've said before, it would be nice if we only called
parallel_vacuum_main() once per VACUUM operation (and not once per
"portion"), but that's a harder and more invasive change. But I don't
think you necessarily have to go that far for it to make sense to have
two bools. Having two might allow you to make both of them immutable,
which is useful.

> Regarding tests, I’d like to add tests to check if a vacuum with
> multiple index scans (i.g., due to small maintenance_work_mem) works
> fine. But a problem is that we need at least about 200,000 garbage
> tuples to perform index scan twice even with the minimum
> maintenance_work_mem. Which takes a time to load tuples.

Maybe that's okay. Do you notice that it takes a lot longer now? I did
try to keep the runtime down when I committed the fixup to the
parallel VACUUM related bug.

-- 
Peter Geoghegan

  14. Re: parallel vacuum comments

    Peter Geoghegan <pg@bowt.ie> — 2021-11-04T21:25:20Z

    On Thu, Nov 4, 2021 at 12:42 PM Peter Geoghegan <pg@bowt.ie> wrote:
> Since "The leader process alone processes all parallel-safe indexes in
> the case where no workers are launched" (no change there), I wonder:
> how does the leader *know* that it's the leader (and so can always
> process any indexes) inside its call to
> lazy_parallel_process_indexes()? Or, does the leader actually process
> all indexes inside lazy_serial_process_indexes() in this edge case?
> (The edge case where a parallel VACUUM has no workers at all, because
> they couldn't be launched by the core parallel infrastructure.)

I think that I might see a related problem. But I'm not sure, so I'll just ask:

> +   /* Set data required for parallel index vacuum or cleanup */
> +   prepare_parallel_index_processing(vacrel, vacuum);
> +
> +   /* Reset the parallel index processing counter */
> +   pg_atomic_write_u32(&(lps->lvshared->idx), 0);
> +
>     /* Setup the shared cost-based vacuum delay and launch workers */
>     if (nworkers > 0)
>     {
> +       /* Reinitialize the parallel context to relaunch parallel workers */
>         if (vacrel->num_index_scans > 0)
> -       {
> -           /* Reset the parallel index processing counter */
> -           pg_atomic_write_u32(&(lps->lvshared->idx), 0);
> -
> -           /* Reinitialize the parallel context to relaunch parallel workers */
>             ReinitializeParallelDSM(lps->pcxt);
> -       }

Is it okay that we don't call ReinitializeParallelDSM() just because
"nworkers == 0" this time around? I notice that there is a wait for
"nworkers_launched" workers to finish parallel processing, at the top
of ReinitializeParallelDSM(). I can see why the
"vacrel->num_index_scans > 0" test is okay, but I can't see why the
"nworkers == 0" test is okay.

I just want to be sure that we're not somehow relying on seeing state
in shared memory (in the LVSharedIndStats array) in all cases, but
finding that it is not actually there in certain rare edge cases.
Maybe this didn't matter before, because the leader didn't expect to
find this information in shared memory in any case. But that is
changed by your patch, of course, so it's something to be concerned
about.

--
Peter Geoghegan

  15. RE: parallel vacuum comments

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-11-05T02:15:23Z

    On Thur, Nov 4, 2021 1:25 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Wed, Nov 3, 2021 at 1:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Nov 2, 2021 at 11:17 AM Masahiko Sawada<sawada.mshk@gmail.com> wrote:
> > >
> > > On Tue, Nov 2, 2021 at 5:57 AM Peter Geoghegan <pg@bowt.ie> wrote:
> > > >
> > >
> > > > Rather than inventing PARALLEL_VACUUM_KEY_INDVAC_CHECK (just for
> > > > assert-enabled builds), we should invent PARALLEL_VACUUM_STATS --
> > > > a dedicated shmem area for the array of LVSharedIndStats (no more
> > > > storing LVSharedIndStats entries at the end of the LVShared space
> > > > in an ad-hoc, type unsafe way). There should be one array element
> > > > for each and every index -- even those indexes where parallel
> > > > index vacuuming is unsafe or not worthwhile (unsure if avoiding
> > > > parallel processing for "not worthwhile" indexes actually makes
> > > > sense, BTW). We can then get rid of the bitmap/IndStatsIsNull()
> > > > stuff entirely. We'd also add new per-index state fields to
> > > > LVSharedIndStats itself. We could directly record the status of
> > > > each index (e.g., parallel unsafe, amvacuumcleanup processing
> > > > done, ambulkdelete processing done) explicitly. All code could
> > > > safely subscript the LVSharedIndStats array directly, using idx
> > > > style integers. That seems far more robust and consistent.
> > >
> > > Sounds good.
> > >
> > > During the development, I wrote the patch while considering using
> > > fewer shared memory but it seems that it brought complexity (and
> > > therefore the bug). It would not be harmful even if we allocate
> > > index statistics on DSM for unsafe indexes and “not worthwhile"
> > > indexes in practice.
> > >
> >
> > If we want to allocate index stats for all indexes in DSM then why not
> > consider it on the lines of buf/wal_usage means tack those via
> > LVParallelState? And probably replace bitmap with an array of bools
> > that indicates which indexes can be skipped by the parallel worker.
> >
> 
> I've attached a draft patch. The patch incorporated all comments from Andres
> except for the last comment that moves parallel related code to another file.
> I'd like to discuss how we split vacuumlazy.c.

Hi,

I was recently reading the parallel vacuum code, and I think the patch can
bring a certain improvement.

Here are a few minor comments about it.

1)

+	 * Reset all index status back to invalid (while checking that we have
+	 * processed all indexes).
+	 */
+	for (int i = 0; i < vacrel->nindexes; i++)
+	{
+		LVSharedIndStats *stats = &(lps->lvsharedindstats[i]);
+
+		Assert(stats->status == INDVAC_STATUS_COMPLETED);
+		stats->status = INDVAC_STATUS_INITIAL;
+	}

Do you think it might be clearer to report an error here ?

2)

+prepare_parallel_index_processing(LVRelState *vacrel, bool vacuum)

For the second paramater 'vacuum'. Would it be clearer if we pass a
LVIndVacStatus type instead of the boolean value ?

Best regards,
Hou zj

  16. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-05T05:28:11Z

    On Fri, Nov 5, 2021 at 4:42 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Wed, Nov 3, 2021 at 10:25 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > I've attached a draft patch. The patch incorporated all comments from
> > Andres except for the last comment that moves parallel related code to
> > another file. I'd like to discuss how we split vacuumlazy.c.
>
> This looks great!
>
> I wonder if this is okay, though:
>
> >     /* Process the indexes that can be processed by only leader process */
> > -   do_serial_processing_for_unsafe_indexes(vacrel, lps->lvshared);
> > +   lazy_serial_process_indexes(vacrel);
> >
> >     /*
> > -    * Join as a parallel worker.  The leader process alone processes all the
> > -    * indexes in the case where no workers are launched.
> > +    * Join as a parallel worker.  The leader process alone processes all
> > +    * parallel-safe indexes in the case where no workers are launched.
> >      */
> > -   do_parallel_processing(vacrel, lps->lvshared);
> > +   lazy_parallel_process_indexes(vacrel, lps->lvshared, vacrel->lps->lvsharedindstats);
> >
> >     /*
> >      * Next, accumulate buffer and WAL usage.  (This must wait for the workers
> > @@ -2786,6 +2734,18 @@ do_parallel_vacuum_or_cleanup(LVRelState *vacrel, int nworkers)
> >             InstrAccumParallelQuery(&lps->buffer_usage[i], &lps->wal_usage[i]);
> >     }
>
> Since "The leader process alone processes all parallel-safe indexes in
> the case where no workers are launched" (no change there), I wonder:
> how does the leader *know* that it's the leader (and so can always
> process any indexes) inside its call to
> lazy_parallel_process_indexes()? Or, does the leader actually process
> all indexes inside lazy_serial_process_indexes() in this edge case?
> (The edge case where a parallel VACUUM has no workers at all, because
> they couldn't be launched by the core parallel infrastructure.)

lazy_serial_process_indexes() handles only parallel-unsafe (i.g.,
non-parallel-supported or too small indexes) indexes whereas
lazy_parallel_process_indexes() does that only parallel-safe indexes.
Therefore in the edge case, the leader process all indexes by using
both functions.

>
> One small thing: the new "LVSharedIndStats.parallel_safe" field seems
> to be slightly misnamed. Isn't it more like
> "LVSharedIndStats.parallel_workers_can_process"? The index might
> actually be parallel safe *in principle*, while nevertheless being
> deliberately skipped over by workers due to a deliberate up-front
> choice made earlier, in compute_parallel_vacuum_workers(). Most
> obvious example of this is the choice to not use parallelism for a
> smaller index (say a partial index whose size is <
> min_parallel_index_scan_size).

Agreed.

>
> Another small thing, which is closely related to the last one:
>
> >  typedef struct LVSharedIndStats
> >  {
> > -   bool        updated;        /* are the stats updated? */
> > +   LVIndVacStatus status;
> > +
> > +   /*
> > +    * True if both leader and worker can process the index, otherwise only
> > +    * leader can process it.
> > +    */
> > +   bool    parallel_safe;
> > +
> > +   bool    istat_updated;      /* are the stats updated? */
> >     IndexBulkDeleteResult istat;
> >  } LVSharedIndStats;
>
> It would be nice to point out that the new
> "LVSharedIndStats.parallel_safe" field (or whatever we end up calling
> it) had comments that pointed out that it isn't a fixed thing for the
> entire VACUUM operation -- it's only fixed for an individual call to
> parallel_lazy_vacuum_or_cleanup_all_indexes() (i.e., it's only fixed
> for the "ambulkdelete portion" or the "amvacuumcleanup portion" of the
> entire VACUUM).

Agreed.

>
> Alternatively, you could just have two booleans, I think. You know,
> one for the "ambulkdelete portion", another for the "amvacuumcleanup
> portion". As I've said before, it would be nice if we only called
> parallel_vacuum_main() once per VACUUM operation (and not once per
> "portion"), but that's a harder and more invasive change. But I don't
> think you necessarily have to go that far for it to make sense to have
> two bools. Having two might allow you to make both of them immutable,
> which is useful.

If we want to make booleans immutable, we need three booleans since
parallel index cleanup behaves differently depending on whether
bulk-deletion has been called once. Anyway, if I understand your
suggestion correctly, it probably means to have booleans corresponding
to VACUUM_OPTION_PARALLEL_XXX flags. Does the worker itself need to
decide whether to skip conditionally-parallel-index-cleanup-safe
indexes?

>
> > Regarding tests, I’d like to add tests to check if a vacuum with
> > multiple index scans (i.g., due to small maintenance_work_mem) works
> > fine. But a problem is that we need at least about 200,000 garbage
> > tuples to perform index scan twice even with the minimum
> > maintenance_work_mem. Which takes a time to load tuples.
>
> Maybe that's okay. Do you notice that it takes a lot longer now? I did
> try to keep the runtime down when I committed the fixup to the
> parallel VACUUM related bug.

It took 6s on my laptop (was 400ms).

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  17. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-05T05:45:39Z

    On Fri, Nov 5, 2021 at 6:25 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Thu, Nov 4, 2021 at 12:42 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > Since "The leader process alone processes all parallel-safe indexes in
> > the case where no workers are launched" (no change there), I wonder:
> > how does the leader *know* that it's the leader (and so can always
> > process any indexes) inside its call to
> > lazy_parallel_process_indexes()? Or, does the leader actually process
> > all indexes inside lazy_serial_process_indexes() in this edge case?
> > (The edge case where a parallel VACUUM has no workers at all, because
> > they couldn't be launched by the core parallel infrastructure.)
>
> I think that I might see a related problem. But I'm not sure, so I'll just ask:
>
> > +   /* Set data required for parallel index vacuum or cleanup */
> > +   prepare_parallel_index_processing(vacrel, vacuum);
> > +
> > +   /* Reset the parallel index processing counter */
> > +   pg_atomic_write_u32(&(lps->lvshared->idx), 0);
> > +
> >     /* Setup the shared cost-based vacuum delay and launch workers */
> >     if (nworkers > 0)
> >     {
> > +       /* Reinitialize the parallel context to relaunch parallel workers */
> >         if (vacrel->num_index_scans > 0)
> > -       {
> > -           /* Reset the parallel index processing counter */
> > -           pg_atomic_write_u32(&(lps->lvshared->idx), 0);
> > -
> > -           /* Reinitialize the parallel context to relaunch parallel workers */
> >             ReinitializeParallelDSM(lps->pcxt);
> > -       }
>
> Is it okay that we don't call ReinitializeParallelDSM() just because
> "nworkers == 0" this time around? I notice that there is a wait for
> "nworkers_launched" workers to finish parallel processing, at the top
> of ReinitializeParallelDSM(). I can see why the
> "vacrel->num_index_scans > 0" test is okay, but I can't see why the
> "nworkers == 0" test is okay.
>
> I just want to be sure that we're not somehow relying on seeing state
> in shared memory (in the LVSharedIndStats array) in all cases, but
> finding that it is not actually there in certain rare edge cases.
> Maybe this didn't matter before, because the leader didn't expect to
> find this information in shared memory in any case. But that is
> changed by your patch, of course, so it's something to be concerned
> about.

If we launch workers (i.g., nworkers > 0), we wait for these workers
to finish after processing all indexes (see we call
WaitForParallelWorkersToFinish() after lazy_parallel_process_indexes).
So it's guaranteed that all workers finished at the end
ofparallel_lazy_vacuum_or_cleanup_all_indexes().  So even in the
second call to this function, we don't need to wait for
"nworkers_launched" workers who previously were running to finish.
Does it make sense?

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  18. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-09T12:53:40Z

    On Fri, Nov 5, 2021 at 4:00 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2021-11-01 10:44:34 +0900, Masahiko Sawada wrote:
> > On Sun, Oct 31, 2021 at 6:21 AM Andres Freund <andres@anarazel.de> wrote:
> > >   But even though we have this space optimized bitmap thing, we actually need
> > >   more memory allocated for each index, making this whole thing pointless.
> >
> > Right. But is better to change to use booleans?
>
> It seems very clearly better to me. We shouldn't use complicated stuff like
>
> #define SizeOfLVShared (offsetof(LVShared, bitmap) + sizeof(bits8))
> #define GetSharedIndStats(s) \
>         ((LVSharedIndStats *)((char *)(s) + ((LVShared *)(s))->offset))
> #define IndStatsIsNull(s, i) \
>         (!(((LVShared *)(s))->bitmap[(i) >> 3] & (1 << ((i) & 0x07))))
>
> when there's reason / benefit.
>
>
> > > - Imo it's pretty confusing to have functions like
> > >   lazy_parallel_vacuum_indexes() (in 13, renamed in 14) that "Perform index
> > >   vacuum or index cleanup with parallel workers.", based on
> > >   lps->lvshared->for_cleanup.
> >
> > Okay. We need to set lps->lvshared->for_cleanup to tell worker do
> > either index vacuum or index cleanup. So it might be better to pass
> > for_cleanup flag down to the functions in addition to setting
> > lps->lvshared->for_cleanup.
>
> Yup.
>
>
> > > - I don't like some of the new names introduced in 14. E.g.
> > >   "do_parallel_processing" is way too generic.
> >
> > I listed the function names that probably needs to be renamed from
> > that perspecti:
> >
> > * do_parallel_processing
> > * do_serial_processing_for_unsafe_indexes
> > * parallel_process_one_index
> >
> > Is there any other function that should be renamed?
>
> parallel_processing_is_safe().
>
> I don't like that there's three different naming patterns for parallel
> things. There's do_parallel_*, there's parallel_, and there's
> (begin|end|compute)_parallel_*.
>
>
> > > - On a higher level, a lot of this actually doesn't seem to belong into
> > >   vacuumlazy.c, but should be somewhere more generic. Pretty much none of this
> > >   code is heap specific.  And vacuumlazy.c is large enough without the parallel
> > >   code.
> >
> > I don't come with an idea to make them more generic. Could you
> > elaborate on that?
>
> To me the the job that the parallel vacuum stuff does isn't really specific to
> heap. Any table AM supporting indexes is going to need to do something pretty
> much like it (it's calling indexam stuff). Most of the stuff in vacuumlazy.c
> is very heap specific - you're not going to be able to reuse lazy_scan_heap()
> or such. Before the parallel vacuum stuff, the index specific code in
> vacuumlazy.c was fairly limited - but now it's a nontrivial amount of code.
>
> Based on a quick look
>   parallel_vacuum_main(), parallel_processing_is_safe(),
>   parallel_stats_for_idx(), end_parallel_vacuum(), begin_parallel_vacuum(),
>   compute_parallel_vacuum_workers(), parallel_process_one_index(),
>   do_serial_processing_for_unsafe_indexes(), do_parallel_processing(),
>   do_parallel_vacuum_or_cleanup(), do_parallel_lazy_cleanup_all_indexes(),
>   do_parallel_lazy_vacuum_all_indexes(),
>
> don't really belong in vacuumlazy.c. but should be in vacuum.c or a new
> file. Of course that requires a bit of work, because of the heavy reliance on
> LVRelState, but afaict there's not really an intrinsic need to use that.

Thanks for your explanation. Understood.

I'll update the patch accordingly.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  19. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-11T02:40:36Z

    On Tue, Nov 9, 2021 at 9:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Nov 5, 2021 at 4:00 AM Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > On 2021-11-01 10:44:34 +0900, Masahiko Sawada wrote:
> > > On Sun, Oct 31, 2021 at 6:21 AM Andres Freund <andres@anarazel.de> wrote:
> > > >   But even though we have this space optimized bitmap thing, we actually need
> > > >   more memory allocated for each index, making this whole thing pointless.
> > >
> > > Right. But is better to change to use booleans?
> >
> > It seems very clearly better to me. We shouldn't use complicated stuff like
> >
> > #define SizeOfLVShared (offsetof(LVShared, bitmap) + sizeof(bits8))
> > #define GetSharedIndStats(s) \
> >         ((LVSharedIndStats *)((char *)(s) + ((LVShared *)(s))->offset))
> > #define IndStatsIsNull(s, i) \
> >         (!(((LVShared *)(s))->bitmap[(i) >> 3] & (1 << ((i) & 0x07))))
> >
> > when there's reason / benefit.
> >
> >
> > > > - Imo it's pretty confusing to have functions like
> > > >   lazy_parallel_vacuum_indexes() (in 13, renamed in 14) that "Perform index
> > > >   vacuum or index cleanup with parallel workers.", based on
> > > >   lps->lvshared->for_cleanup.
> > >
> > > Okay. We need to set lps->lvshared->for_cleanup to tell worker do
> > > either index vacuum or index cleanup. So it might be better to pass
> > > for_cleanup flag down to the functions in addition to setting
> > > lps->lvshared->for_cleanup.
> >
> > Yup.
> >
> >
> > > > - I don't like some of the new names introduced in 14. E.g.
> > > >   "do_parallel_processing" is way too generic.
> > >
> > > I listed the function names that probably needs to be renamed from
> > > that perspecti:
> > >
> > > * do_parallel_processing
> > > * do_serial_processing_for_unsafe_indexes
> > > * parallel_process_one_index
> > >
> > > Is there any other function that should be renamed?
> >
> > parallel_processing_is_safe().
> >
> > I don't like that there's three different naming patterns for parallel
> > things. There's do_parallel_*, there's parallel_, and there's
> > (begin|end|compute)_parallel_*.
> >
> >
> > > > - On a higher level, a lot of this actually doesn't seem to belong into
> > > >   vacuumlazy.c, but should be somewhere more generic. Pretty much none of this
> > > >   code is heap specific.  And vacuumlazy.c is large enough without the parallel
> > > >   code.
> > >
> > > I don't come with an idea to make them more generic. Could you
> > > elaborate on that?
> >
> > To me the the job that the parallel vacuum stuff does isn't really specific to
> > heap. Any table AM supporting indexes is going to need to do something pretty
> > much like it (it's calling indexam stuff). Most of the stuff in vacuumlazy.c
> > is very heap specific - you're not going to be able to reuse lazy_scan_heap()
> > or such. Before the parallel vacuum stuff, the index specific code in
> > vacuumlazy.c was fairly limited - but now it's a nontrivial amount of code.
> >
> > Based on a quick look
> >   parallel_vacuum_main(), parallel_processing_is_safe(),
> >   parallel_stats_for_idx(), end_parallel_vacuum(), begin_parallel_vacuum(),
> >   compute_parallel_vacuum_workers(), parallel_process_one_index(),
> >   do_serial_processing_for_unsafe_indexes(), do_parallel_processing(),
> >   do_parallel_vacuum_or_cleanup(), do_parallel_lazy_cleanup_all_indexes(),
> >   do_parallel_lazy_vacuum_all_indexes(),
> >
> > don't really belong in vacuumlazy.c. but should be in vacuum.c or a new
> > file. Of course that requires a bit of work, because of the heavy reliance on
> > LVRelState, but afaict there's not really an intrinsic need to use that.
>
> Thanks for your explanation. Understood.
>
> I'll update the patch accordingly.
>

I've attached a draft patch that refactors parallel vacuum and
separates parallel-vacuum-related code to new file vacuumparallel.c.
After discussion, I'll divide the patch into logical chunks.

What I'm not convinced yet in this patch is that vacuum.c,
vacuumlazy.c and vacuumparallel.c depend on the data structure to
store dead tuples (now called VacDeadTuples, was LVDeadTuples). I
thought that it might be better to separate it so that a table AM can
use another type of data structure to store dead tuples. But since I
think it may bring complexity, currently a table AM has to use
VacDeadTuples in order to use the parallel vacuum. Feedback is very
welcome.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  20. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-11T09:38:15Z

    On Thu, Nov 11, 2021 at 8:11 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've attached a draft patch that refactors parallel vacuum and
> separates parallel-vacuum-related code to new file vacuumparallel.c.
> After discussion, I'll divide the patch into logical chunks.
>
> What I'm not convinced yet in this patch is that vacuum.c,
> vacuumlazy.c and vacuumparallel.c depend on the data structure to
> store dead tuples (now called VacDeadTuples, was LVDeadTuples). I
> thought that it might be better to separate it so that a table AM can
> use another type of data structure to store dead tuples. But since I
> think it may bring complexity, currently a table AM has to use
> VacDeadTuples in order to use the parallel vacuum.
>

I think it might be better to attempt doing anything to make it
generic for tableAM in a separate patch if that is required.

Few questions/comments:
=====================
1. There are three different structures PVState,
ParallelVacuumContext, and ParallelVacuumCtl to maintain the parallel
vacuum state. Can't we merge PVState and ParallelVacuumCtl? Also, I
see that most of the fields of PVState are there in
ParallelVacuumContext except for error info fields, does it makes
sense to directly use PVState instead? Also, it would be better to
write some more comments atop each structure to explain its usage.

2. In vacuum.c, the function names doesn't match the names in their
corresponding function header comments.

3.
+ INDVAC_STATUS_COMPLETED,
+} PVIndVacStatus;

The comma is not required after the last value of enum.

-- 
With Regards,
Amit Kapila.

  21. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-15T08:30:59Z

    On Thu, Nov 11, 2021 at 6:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Nov 11, 2021 at 8:11 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached a draft patch that refactors parallel vacuum and
> > separates parallel-vacuum-related code to new file vacuumparallel.c.
> > After discussion, I'll divide the patch into logical chunks.
> >
> > What I'm not convinced yet in this patch is that vacuum.c,
> > vacuumlazy.c and vacuumparallel.c depend on the data structure to
> > store dead tuples (now called VacDeadTuples, was LVDeadTuples). I
> > thought that it might be better to separate it so that a table AM can
> > use another type of data structure to store dead tuples. But since I
> > think it may bring complexity, currently a table AM has to use
> > VacDeadTuples in order to use the parallel vacuum.
> >
>
> I think it might be better to attempt doing anything to make it
> generic for tableAM in a separate patch if that is required.

You mean to refactor relation_vacuum table AM API too? Currently,
relation_vacuum API is responsible for whole vacuum operation and
there is no room for the core doing anything during vacuum. So
probably it doesn’t make sense to have a table AM API for parallel
vacuum.

>
> Few questions/comments:
> =====================
> 1. There are three different structures PVState,
> ParallelVacuumContext, and ParallelVacuumCtl to maintain the parallel
> vacuum state. Can't we merge PVState and ParallelVacuumCtl? Also, I
> see that most of the fields of PVState are there in
> ParallelVacuumContext except for error info fields, does it makes
> sense to directly use PVState instead?

Agreed.

>  Also, it would be better to
> write some more comments atop each structure to explain its usage.

Agreed.

>
> 2. In vacuum.c, the function names doesn't match the names in their
> corresponding function header comments.

Will fix.

>
> 3.
> + INDVAC_STATUS_COMPLETED,
> +} PVIndVacStatus;
>
> The comma is not required after the last value of enum.

Will fix.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  22. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-15T11:03:58Z

    On Mon, Nov 15, 2021 at 2:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Nov 11, 2021 at 6:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Nov 11, 2021 at 8:11 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > I've attached a draft patch that refactors parallel vacuum and
> > > separates parallel-vacuum-related code to new file vacuumparallel.c.
> > > After discussion, I'll divide the patch into logical chunks.
> > >
> > > What I'm not convinced yet in this patch is that vacuum.c,
> > > vacuumlazy.c and vacuumparallel.c depend on the data structure to
> > > store dead tuples (now called VacDeadTuples, was LVDeadTuples). I
> > > thought that it might be better to separate it so that a table AM can
> > > use another type of data structure to store dead tuples. But since I
> > > think it may bring complexity, currently a table AM has to use
> > > VacDeadTuples in order to use the parallel vacuum.
> > >
> >
> > I think it might be better to attempt doing anything to make it
> > generic for tableAM in a separate patch if that is required.
>
> You mean to refactor relation_vacuum table AM API too? Currently,
> relation_vacuum API is responsible for whole vacuum operation and
> there is no room for the core doing anything during vacuum. So
> probably it doesn’t make sense to have a table AM API for parallel
> vacuum.
>

No, I intend to say that let's not do anything for it as of now. It is
not clear what a generic structure for it should be and whether AM's
need anything like that. As the current structure is specific to heap,
it might make sense to declare it in heapam.h as we are doing for
function heap_vacuum_rel(), and additionally, you might want to
include heap in that structure name to be more explicit.

-- 
With Regards,
Amit Kapila.

  23. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-15T12:43:53Z

    On Mon, Nov 15, 2021 at 8:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Nov 15, 2021 at 2:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Nov 11, 2021 at 6:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Nov 11, 2021 at 8:11 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > I've attached a draft patch that refactors parallel vacuum and
> > > > separates parallel-vacuum-related code to new file vacuumparallel.c.
> > > > After discussion, I'll divide the patch into logical chunks.
> > > >
> > > > What I'm not convinced yet in this patch is that vacuum.c,
> > > > vacuumlazy.c and vacuumparallel.c depend on the data structure to
> > > > store dead tuples (now called VacDeadTuples, was LVDeadTuples). I
> > > > thought that it might be better to separate it so that a table AM can
> > > > use another type of data structure to store dead tuples. But since I
> > > > think it may bring complexity, currently a table AM has to use
> > > > VacDeadTuples in order to use the parallel vacuum.
> > > >
> > >
> > > I think it might be better to attempt doing anything to make it
> > > generic for tableAM in a separate patch if that is required.
> >
> > You mean to refactor relation_vacuum table AM API too? Currently,
> > relation_vacuum API is responsible for whole vacuum operation and
> > there is no room for the core doing anything during vacuum. So
> > probably it doesn’t make sense to have a table AM API for parallel
> > vacuum.
> >
>
> No, I intend to say that let's not do anything for it as of now. It is
> not clear what a generic structure for it should be and whether AM's
> need anything like that. As the current structure is specific to heap,
> it might make sense to declare it in heapam.h as we are doing for
> function heap_vacuum_rel(), and additionally, you might want to
> include heap in that structure name to be more explicit.

Thanks for your clarification. I agreed.

I'll update an updated patch tomorrow.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  24. RE: parallel vacuum comments

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-11-16T02:38:29Z

    On Thur, Nov 11, 2021 10:41 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've attached a draft patch that refactors parallel vacuum and separates
> parallel-vacuum-related code to new file vacuumparallel.c.
> After discussion, I'll divide the patch into logical chunks.

Hi.

I noticed few minor issues in the patch.

1)
+		/*
+		 * Parallel unsafe indexes can be processed only by leader (these are
+		 * processed in lazy_serial_process_indexes() by leader.
+		 */

It seems the function name in the comments should be serial_vacuum_unsafe_indexes

2)
+		stats->parallel_workers_can_process =
+			index_parallel_vacuum_is_safe(pvc->indrels[i],
+										  pvc->num_index_scans,
+										  bulkdel);

The function index_parallel_vacuum_is_safe also return false for the
index < min_parallel_index_scan_size cutoff which seems parallel safe. So,
maybe we can rename the function to xxx_worker_can_process() ?

Best regards,
Hou zj

  25. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-16T05:53:18Z

    On Tue, Nov 16, 2021 at 11:38 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Thur, Nov 11, 2021 10:41 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > I've attached a draft patch that refactors parallel vacuum and separates
> > parallel-vacuum-related code to new file vacuumparallel.c.
> > After discussion, I'll divide the patch into logical chunks.
>
> Hi.
>
> I noticed few minor issues in the patch.
>
> 1)
> +               /*
> +                * Parallel unsafe indexes can be processed only by leader (these are
> +                * processed in lazy_serial_process_indexes() by leader.
> +                */
>
> It seems the function name in the comments should be serial_vacuum_unsafe_indexes
>
> 2)
> +               stats->parallel_workers_can_process =
> +                       index_parallel_vacuum_is_safe(pvc->indrels[i],
> +                                                                                 pvc->num_index_scans,
> +                                                                                 bulkdel);
>
> The function index_parallel_vacuum_is_safe also return false for the
> index < min_parallel_index_scan_size cutoff which seems parallel safe. So,
> maybe we can rename the function to xxx_worker_can_process() ?

Thank you for the comments!

I've incorporated these comments and attached an updated patch.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  26. RE: parallel vacuum comments

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-11-19T02:25:29Z

    On Tues, Nov 16, 2021 1:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've incorporated these comments and attached an updated patch.

Thanks for updating the patch.
I read the latest patch and have few comments.

1)
+/*
+ *	lazy_vacuum_one_index() -- vacuum index relation.
...
+IndexBulkDeleteResult *
+vacuum_one_index(IndexVacuumInfo *ivinfo, IndexBulkDeleteResult *istat,


+ *	vac_cleanup_one_index() -- do post-vacuum cleanup for index relation.
...
+IndexBulkDeleteResult *
+cleanup_one_index(IndexVacuumInfo *ivinfo, IndexBulkDeleteResult *istat)

The above function names seem different from the name mentioned in the function
header.

2)
 static void vacuum_error_callback(void *arg);

I noticed the patch changed the parallel worker's error callback function to
parallel_index_vacuum_error_callback(). The error message in new callback
function seems a little different from the old one, was it intentional ?


3)

+	/*
+	 * Reset all index status back to invalid (while checking that we have
+	 * processed all indexes).
+	 */
+	for (int i = 0; i < pvs->nindexes; i++)
+	{
+		PVIndStats *stats = &(pvs->indstats[i]);
+
+		Assert(stats->status == INDVAC_STATUS_COMPLETED);
+		stats->status = INDVAC_STATUS_INITIAL;
+	}

Would it be safer if we report an error if any index's status is not
INDVAC_STATUS_COMPLETED ?

4)

Just a personal suggestion for the parallel related function name. Since Andres
wanted a uniform naming pattern. Mabe we can rename the following functions:

end|begin_parallel_vacuum => parallel_vacuum_end|begin
perform_parallel_index_bulkdel|cleanup => parallel_vacuum_index_bulkdel|cleanup

So that all the parallel related functions' name is like parallel_vacuum_xxx.

Best regards,
Hou zj

  27. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-22T04:48:13Z

    On Fri, Nov 19, 2021 at 7:55 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Tues, Nov 16, 2021 1:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > I've incorporated these comments and attached an updated patch.
>
>
> 2)
>  static void vacuum_error_callback(void *arg);
>
> I noticed the patch changed the parallel worker's error callback function to
> parallel_index_vacuum_error_callback(). The error message in new callback
> function seems a little different from the old one, was it intentional ?
>

One more point related to this is that it seems a new callback will be
invoked only by parallel workers, so the context displayed during
parallel vacuum will be different based on if the error happens during
processing by leader or worker. I think if done correctly this would
be an improvement over what we have now but isn't it better to do this
change as a separate patch?

>
> 4)
>
> Just a personal suggestion for the parallel related function name. Since Andres
> wanted a uniform naming pattern. Mabe we can rename the following functions:
>
> end|begin_parallel_vacuum => parallel_vacuum_end|begin
> perform_parallel_index_bulkdel|cleanup => parallel_vacuum_index_bulkdel|cleanup
>
> So that all the parallel related functions' name is like parallel_vacuum_xxx.
>

BTW, do we really need functions
perform_parallel_index_bulkdel|cleanup? Both do some minimal
assignments and then call parallel_vacuum_all_indexes() and there is
just one caller of each. Isn't it better to just do those assignments
in the caller and directly call parallel_vacuum_all_indexes()?

In general, we are not following the convention to start the function
names with parallel_* at other places so I think we should consider
such a convention on case to case basis. In this case, if we can get
rid of perform_parallel_index_bulkdel|cleanup then we probably don't
need such a renaming.

-- 
With Regards,
Amit Kapila.

  28. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-22T09:35:09Z

    On Tue, Nov 16, 2021 at 11:23 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've incorporated these comments and attached an updated patch.
>

Review comments:
================
1.
index_can_participate_parallel_vacuum()
{
..
+ /*
+ * Not safe, if the index supports parallel cleanup conditionally,
+ * but we have already processed the index (for bulkdelete).  See the
+ * comments for option VACUUM_OPTION_PARALLEL_COND_CLEANUP to know
+ * when indexes support parallel cleanup conditionally.
+ */
+ if (num_index_scans > 0 &&
+ ((vacoptions & VACUUM_OPTION_PARALLEL_COND_CLEANUP) != 0))
..
}

IIRC, we do this to avoid the need to invoke worker when parallel
cleanup doesn't need to scan the index which means the work required
to be performed by a worker would be minimal. If so, maybe we can
write that in comments here or with
VACUUM_OPTION_PARALLEL_COND_CLEANUP.

If the above understanding is correct then is it correct to check
num_index_scans here? AFAICS, this value is incremented in
parallel_vacuum_all_indexes irrespective of whether it is invoked for
bulk delete or clean up. OTOH, previously, this was done based on
first_time variable which was in turn set based on
vacrel->num_index_scans and that is incremented in
lazy_vacuum_all_indexes(both in serial and parallel case).

2. The structure ParallelVacuumState contains both PVIndVacStatus and
PVIndStats. Considering PVIndVacStatus is already present in
PVIndStats, does ParallelVacuumState need to have both?

3. Why ParallelVacuumCtl is declared in vacuum.h? It appears to be
only used in one function begin_parallel_vacuum, can't we just declare
in vacuumparallel.c? As it is only required for one function and it is
not that the number of individual parameters will be too huge, can't
we do without having that structure.


-- 
With Regards,
Amit Kapila.

  29. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-24T01:36:41Z

    On Mon, Nov 22, 2021 at 6:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Nov 16, 2021 at 11:23 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've incorporated these comments and attached an updated patch.
> >
>
> Review comments:
> ================
> 1.
> index_can_participate_parallel_vacuum()
> {
> ..
> + /*
> + * Not safe, if the index supports parallel cleanup conditionally,
> + * but we have already processed the index (for bulkdelete).  See the
> + * comments for option VACUUM_OPTION_PARALLEL_COND_CLEANUP to know
> + * when indexes support parallel cleanup conditionally.
> + */
> + if (num_index_scans > 0 &&
> + ((vacoptions & VACUUM_OPTION_PARALLEL_COND_CLEANUP) != 0))
> ..
> }
>
> IIRC, we do this to avoid the need to invoke worker when parallel
> cleanup doesn't need to scan the index which means the work required
> to be performed by a worker would be minimal. If so, maybe we can
> write that in comments here or with
> VACUUM_OPTION_PARALLEL_COND_CLEANUP.

Right. Will add the comments.

> If the above understanding is correct then is it correct to check
> num_index_scans here? AFAICS, this value is incremented in
> parallel_vacuum_all_indexes irrespective of whether it is invoked for
> bulk delete or clean up. OTOH, previously, this was done based on
> first_time variable which was in turn set based on
> vacrel->num_index_scans and that is incremented in
> lazy_vacuum_all_indexes(both in serial and parallel case).

You're right. That's wrong to increment num_index_scans also when
vacuumcleanup. It should be incremented only when bulkdelete. Perhaps,
the caller (i.g., table AM) should pass num_index_scans to parallel
vacuum code? I initially thought that ParallelVacuumState can have
num_index_scans and increment it only when parallel bulk-deletion. But
if we do that we will end up having the same thing in two places:
ParallelVacuumState and LVRelState. It would be clearer if we maintain
num_index_scan in LVRelState and pass it to parallel index vacuum when
calling to parallel index bulk-deletion or cleanup. On the other hand,
the downside would be that there is a possibility that a table AM
passes the wrong num_index_scans. Probably it’s also a valid argument
that since if a table AM is capable of parallel index vacuum, it’s
better to outsource index bulkdelete/cleanup to parallel index vacuum
through a whole vacuum operation, it’d be better to have
ParallelVacuumState maintain num_index_scans.

>
> 2. The structure ParallelVacuumState contains both PVIndVacStatus and
> PVIndStats. Considering PVIndVacStatus is already present in
> PVIndStats, does ParallelVacuumState need to have both?

"PVIndVacStatus status” of ParallelVacuumState is used by the worker
in the error callback function,
parallel_index_vacuum_error_callback(), in order to know the status of
the index vacuum that the worker is working on. I think that without
PVIndVacStatus, the worker needs to have the index of the PVIndStats
array in order to get the status by like
errinfo->indstats[idx]->status. Do you prefer to do that?

> 3. Why ParallelVacuumCtl is declared in vacuum.h? It appears to be
> only used in one function begin_parallel_vacuum, can't we just declare
> in vacuumparallel.c?

ParallelVacuumCtl is a struct to begin the parallel vacuum and
therefore is expected to be passed by table AM. If we declare it in
vacuumparallel.c a table AM (e.g., vacuumlazy.c) cannot use it, no?

> As it is only required for one function and it is
> not that the number of individual parameters will be too huge, can't
> we do without having that structure.

Yes, we can do that without having that structure. I was a bit
concerned that there are already 7 arguments.


Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  30. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-24T02:12:57Z

    On Mon, Nov 22, 2021 at 1:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Nov 19, 2021 at 7:55 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Tues, Nov 16, 2021 1:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > I've incorporated these comments and attached an updated patch.
> >
> >
> > 2)
> >  static void vacuum_error_callback(void *arg);
> >
> > I noticed the patch changed the parallel worker's error callback function to
> > parallel_index_vacuum_error_callback(). The error message in new callback
> > function seems a little different from the old one, was it intentional ?
> >
>
> One more point related to this is that it seems a new callback will be
> invoked only by parallel workers, so the context displayed during
> parallel vacuum will be different based on if the error happens during
> processing by leader or worker. I think if done correctly this would
> be an improvement over what we have now but isn't it better to do this
> change as a separate patch?

Agreed.

>
> >
> > 4)
> >
> > Just a personal suggestion for the parallel related function name. Since Andres
> > wanted a uniform naming pattern. Mabe we can rename the following functions:
> >
> > end|begin_parallel_vacuum => parallel_vacuum_end|begin
> > perform_parallel_index_bulkdel|cleanup => parallel_vacuum_index_bulkdel|cleanup
> >
> > So that all the parallel related functions' name is like parallel_vacuum_xxx.
> >
>
> BTW, do we really need functions
> perform_parallel_index_bulkdel|cleanup? Both do some minimal
> assignments and then call parallel_vacuum_all_indexes() and there is
> just one caller of each. Isn't it better to just do those assignments
> in the caller and directly call parallel_vacuum_all_indexes()?

The reason why I declare these two functions are: (1) the fields of
ParallelVacuumState are not exposed and (2) bulk-deletion and cleanup
require different arguments (estimated_count is required only by
cleanup).  So if we expose the fields of ParallelVacuumState, the
caller can do those assignments and directly call
parallel_vacuum_all_indexes(). But I'm not sure it's good if those
assignments are the caller's responsibility.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  31. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-24T02:18:07Z

    On Fri, Nov 19, 2021 at 11:25 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Tues, Nov 16, 2021 1:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > I've incorporated these comments and attached an updated patch.
>
> Thanks for updating the patch.
> I read the latest patch and have few comments.

Thank you for the comments! For the comments (2) and (4), I replied in
a separate email to answer your and Amit's comments.

>
> 1)
> +/*
> + *     lazy_vacuum_one_index() -- vacuum index relation.
> ...
> +IndexBulkDeleteResult *
> +vacuum_one_index(IndexVacuumInfo *ivinfo, IndexBulkDeleteResult *istat,
>
>
> + *     vac_cleanup_one_index() -- do post-vacuum cleanup for index relation.
> ...
> +IndexBulkDeleteResult *
> +cleanup_one_index(IndexVacuumInfo *ivinfo, IndexBulkDeleteResult *istat)
>
> The above function names seem different from the name mentioned in the function
> header.

Will fix both.

>
> 3)
>
> +       /*
> +        * Reset all index status back to invalid (while checking that we have
> +        * processed all indexes).
> +        */
> +       for (int i = 0; i < pvs->nindexes; i++)
> +       {
> +               PVIndStats *stats = &(pvs->indstats[i]);
> +
> +               Assert(stats->status == INDVAC_STATUS_COMPLETED);
> +               stats->status = INDVAC_STATUS_INITIAL;
> +       }
>
> Would it be safer if we report an error if any index's status is not
> INDVAC_STATUS_COMPLETED ?

Agreed. It'd be safer since even if some indexes are vacuumed due to a
bug vacuum errored out rather than continue it (and cause index
corruption).

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  32. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-24T02:47:09Z

    On Wed, Nov 24, 2021 at 7:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Nov 19, 2021 at 11:25 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > 3)
> >
> > +       /*
> > +        * Reset all index status back to invalid (while checking that we have
> > +        * processed all indexes).
> > +        */
> > +       for (int i = 0; i < pvs->nindexes; i++)
> > +       {
> > +               PVIndStats *stats = &(pvs->indstats[i]);
> > +
> > +               Assert(stats->status == INDVAC_STATUS_COMPLETED);
> > +               stats->status = INDVAC_STATUS_INITIAL;
> > +       }
> >
> > Would it be safer if we report an error if any index's status is not
> > INDVAC_STATUS_COMPLETED ?
>
> Agreed. It'd be safer since even if some indexes are vacuumed due to a
> bug vacuum errored out rather than continue it (and cause index
> corruption).
>

I think if we want to report an error in this case, we should use elog
as this is an internal error.

-- 
With Regards,
Amit Kapila.

  33. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-24T04:28:06Z

    On Wed, Nov 24, 2021 at 7:07 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Nov 22, 2021 at 6:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Nov 16, 2021 at 11:23 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > I've incorporated these comments and attached an updated patch.
> > >
> >
> > Review comments:
> > ================
> > 1.
> > index_can_participate_parallel_vacuum()
> > {
> > ..
> > + /*
> > + * Not safe, if the index supports parallel cleanup conditionally,
> > + * but we have already processed the index (for bulkdelete).  See the
> > + * comments for option VACUUM_OPTION_PARALLEL_COND_CLEANUP to know
> > + * when indexes support parallel cleanup conditionally.
> > + */
> > + if (num_index_scans > 0 &&
> > + ((vacoptions & VACUUM_OPTION_PARALLEL_COND_CLEANUP) != 0))
> > ..
> > }
> >
> > IIRC, we do this to avoid the need to invoke worker when parallel
> > cleanup doesn't need to scan the index which means the work required
> > to be performed by a worker would be minimal. If so, maybe we can
> > write that in comments here or with
> > VACUUM_OPTION_PARALLEL_COND_CLEANUP.
>
> Right. Will add the comments.
>
> > If the above understanding is correct then is it correct to check
> > num_index_scans here? AFAICS, this value is incremented in
> > parallel_vacuum_all_indexes irrespective of whether it is invoked for
> > bulk delete or clean up. OTOH, previously, this was done based on
> > first_time variable which was in turn set based on
> > vacrel->num_index_scans and that is incremented in
> > lazy_vacuum_all_indexes(both in serial and parallel case).
>
> You're right. That's wrong to increment num_index_scans also when
> vacuumcleanup. It should be incremented only when bulkdelete. Perhaps,
> the caller (i.g., table AM) should pass num_index_scans to parallel
> vacuum code? I initially thought that ParallelVacuumState can have
> num_index_scans and increment it only when parallel bulk-deletion. But
> if we do that we will end up having the same thing in two places:
> ParallelVacuumState and LVRelState. It would be clearer if we maintain
> num_index_scan in LVRelState and pass it to parallel index vacuum when
> calling to parallel index bulk-deletion or cleanup.
>

That sounds reasonable.

> On the other hand,
> the downside would be that there is a possibility that a table AM
> passes the wrong num_index_scans.
>

If that happens then also there will be no problem as such because it
will do some work via worker where it would have been done by the
leader itself. I think it is better to have one source of information
for this as we need to mainly consider whether bulkdelete has been
already performed or not, it doesn't matter whether that is performed
by leader or worker.

>
> >
> > 2. The structure ParallelVacuumState contains both PVIndVacStatus and
> > PVIndStats. Considering PVIndVacStatus is already present in
> > PVIndStats, does ParallelVacuumState need to have both?
>
> "PVIndVacStatus status” of ParallelVacuumState is used by the worker
> in the error callback function,
> parallel_index_vacuum_error_callback(), in order to know the status of
> the index vacuum that the worker is working on. I think that without
> PVIndVacStatus, the worker needs to have the index of the PVIndStats
> array in order to get the status by like
> errinfo->indstats[idx]->status. Do you prefer to do that?
>

As mentioned in my another email to which you agreed that we need to
re-design this callback and do it separately, I think it is better to
consider it separately. So, we can probably remove this parameter from
the main patch as of now.

> > 3. Why ParallelVacuumCtl is declared in vacuum.h? It appears to be
> > only used in one function begin_parallel_vacuum, can't we just declare
> > in vacuumparallel.c?
>
> ParallelVacuumCtl is a struct to begin the parallel vacuum and
> therefore is expected to be passed by table AM. If we declare it in
> vacuumparallel.c a table AM (e.g., vacuumlazy.c) cannot use it, no?
>
> > As it is only required for one function and it is
> > not that the number of individual parameters will be too huge, can't
> > we do without having that structure.
>
> Yes, we can do that without having that structure. I was a bit
> concerned that there are already 7 arguments.
>

Yeah, it is better to have fewer arguments but I don't this number is
big enough to worry. Also, I am not sure about the table AM point as
there is no clear example in front of us which tells how any other
table AM might use it and whether this structure is generic enough. So
I think it might be better to use arguments for this and if we later
find some generic use then we can replace it with structure.

-- 
With Regards,
Amit Kapila.

  34. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-24T04:34:17Z

    On Wed, Nov 24, 2021 at 7:43 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Nov 22, 2021 at 1:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Nov 19, 2021 at 7:55 AM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> >
> > >
> > > 4)
> > >
> > > Just a personal suggestion for the parallel related function name. Since Andres
> > > wanted a uniform naming pattern. Mabe we can rename the following functions:
> > >
> > > end|begin_parallel_vacuum => parallel_vacuum_end|begin
> > > perform_parallel_index_bulkdel|cleanup => parallel_vacuum_index_bulkdel|cleanup
> > >
> > > So that all the parallel related functions' name is like parallel_vacuum_xxx.
> > >
> >
> > BTW, do we really need functions
> > perform_parallel_index_bulkdel|cleanup? Both do some minimal
> > assignments and then call parallel_vacuum_all_indexes() and there is
> > just one caller of each. Isn't it better to just do those assignments
> > in the caller and directly call parallel_vacuum_all_indexes()?
>
> The reason why I declare these two functions are: (1) the fields of
> ParallelVacuumState are not exposed and (2) bulk-deletion and cleanup
> require different arguments (estimated_count is required only by
> cleanup).  So if we expose the fields of ParallelVacuumState, the
> caller can do those assignments and directly call
> parallel_vacuum_all_indexes(). But I'm not sure it's good if those
> assignments are the caller's responsibility.
>

Okay, that makes sense. However, I am still not very comfortable with
the function naming suggested by Hou-San, do you have any thoughts on
that?

-- 
With Regards,
Amit Kapila.

  35. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-24T06:18:59Z

    On Wed, Nov 24, 2021 at 1:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Nov 24, 2021 at 7:07 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Nov 22, 2021 at 6:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Nov 16, 2021 at 11:23 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > I've incorporated these comments and attached an updated patch.
> > > >
> > >
> > > Review comments:
> > > ================
> > > 1.
> > > index_can_participate_parallel_vacuum()
> > > {
> > > ..
> > > + /*
> > > + * Not safe, if the index supports parallel cleanup conditionally,
> > > + * but we have already processed the index (for bulkdelete).  See the
> > > + * comments for option VACUUM_OPTION_PARALLEL_COND_CLEANUP to know
> > > + * when indexes support parallel cleanup conditionally.
> > > + */
> > > + if (num_index_scans > 0 &&
> > > + ((vacoptions & VACUUM_OPTION_PARALLEL_COND_CLEANUP) != 0))
> > > ..
> > > }
> > >
> > > IIRC, we do this to avoid the need to invoke worker when parallel
> > > cleanup doesn't need to scan the index which means the work required
> > > to be performed by a worker would be minimal. If so, maybe we can
> > > write that in comments here or with
> > > VACUUM_OPTION_PARALLEL_COND_CLEANUP.
> >
> > Right. Will add the comments.
> >
> > > If the above understanding is correct then is it correct to check
> > > num_index_scans here? AFAICS, this value is incremented in
> > > parallel_vacuum_all_indexes irrespective of whether it is invoked for
> > > bulk delete or clean up. OTOH, previously, this was done based on
> > > first_time variable which was in turn set based on
> > > vacrel->num_index_scans and that is incremented in
> > > lazy_vacuum_all_indexes(both in serial and parallel case).
> >
> > You're right. That's wrong to increment num_index_scans also when
> > vacuumcleanup. It should be incremented only when bulkdelete. Perhaps,
> > the caller (i.g., table AM) should pass num_index_scans to parallel
> > vacuum code? I initially thought that ParallelVacuumState can have
> > num_index_scans and increment it only when parallel bulk-deletion. But
> > if we do that we will end up having the same thing in two places:
> > ParallelVacuumState and LVRelState. It would be clearer if we maintain
> > num_index_scan in LVRelState and pass it to parallel index vacuum when
> > calling to parallel index bulk-deletion or cleanup.
> >
>
> That sounds reasonable.
>
> > On the other hand,
> > the downside would be that there is a possibility that a table AM
> > passes the wrong num_index_scans.
> >
>
> If that happens then also there will be no problem as such because it
> will do some work via worker where it would have been done by the
> leader itself. I think it is better to have one source of information
> for this as we need to mainly consider whether bulkdelete has been
> already performed or not, it doesn't matter whether that is performed
> by leader or worker.

Agreed.

>
> >
> > >
> > > 2. The structure ParallelVacuumState contains both PVIndVacStatus and
> > > PVIndStats. Considering PVIndVacStatus is already present in
> > > PVIndStats, does ParallelVacuumState need to have both?
> >
> > "PVIndVacStatus status” of ParallelVacuumState is used by the worker
> > in the error callback function,
> > parallel_index_vacuum_error_callback(), in order to know the status of
> > the index vacuum that the worker is working on. I think that without
> > PVIndVacStatus, the worker needs to have the index of the PVIndStats
> > array in order to get the status by like
> > errinfo->indstats[idx]->status. Do you prefer to do that?
> >
>
> As mentioned in my another email to which you agreed that we need to
> re-design this callback and do it separately, I think it is better to
> consider it separately. So, we can probably remove this parameter from
> the main patch as of now.

Yes, I'll remove it from the next version patch. With that, since
parallel vacuum workers don't set errcontext we will need to do
something for that in a separate patch.

>
> > > 3. Why ParallelVacuumCtl is declared in vacuum.h? It appears to be
> > > only used in one function begin_parallel_vacuum, can't we just declare
> > > in vacuumparallel.c?
> >
> > ParallelVacuumCtl is a struct to begin the parallel vacuum and
> > therefore is expected to be passed by table AM. If we declare it in
> > vacuumparallel.c a table AM (e.g., vacuumlazy.c) cannot use it, no?
> >
> > > As it is only required for one function and it is
> > > not that the number of individual parameters will be too huge, can't
> > > we do without having that structure.
> >
> > Yes, we can do that without having that structure. I was a bit
> > concerned that there are already 7 arguments.
> >
>
> Yeah, it is better to have fewer arguments but I don't this number is
> big enough to worry. Also, I am not sure about the table AM point as
> there is no clear example in front of us which tells how any other
> table AM might use it and whether this structure is generic enough. So
> I think it might be better to use arguments for this and if we later
> find some generic use then we can replace it with structure.

Makes sense. Will fix in the next version patch.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  36. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-24T06:45:45Z

    On Wed, Nov 24, 2021 at 1:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Nov 24, 2021 at 7:43 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Nov 22, 2021 at 1:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, Nov 19, 2021 at 7:55 AM houzj.fnst@fujitsu.com
> > > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > >
> > > > 4)
> > > >
> > > > Just a personal suggestion for the parallel related function name. Since Andres
> > > > wanted a uniform naming pattern. Mabe we can rename the following functions:
> > > >
> > > > end|begin_parallel_vacuum => parallel_vacuum_end|begin
> > > > perform_parallel_index_bulkdel|cleanup => parallel_vacuum_index_bulkdel|cleanup
> > > >
> > > > So that all the parallel related functions' name is like parallel_vacuum_xxx.
> > > >
> > >
> > > BTW, do we really need functions
> > > perform_parallel_index_bulkdel|cleanup? Both do some minimal
> > > assignments and then call parallel_vacuum_all_indexes() and there is
> > > just one caller of each. Isn't it better to just do those assignments
> > > in the caller and directly call parallel_vacuum_all_indexes()?
> >
> > The reason why I declare these two functions are: (1) the fields of
> > ParallelVacuumState are not exposed and (2) bulk-deletion and cleanup
> > require different arguments (estimated_count is required only by
> > cleanup).  So if we expose the fields of ParallelVacuumState, the
> > caller can do those assignments and directly call
> > parallel_vacuum_all_indexes(). But I'm not sure it's good if those
> > assignments are the caller's responsibility.
> >
>
> Okay, that makes sense. However, I am still not very comfortable with
> the function naming suggested by Hou-San, do you have any thoughts on
> that?

I personally don't disagree with the names starting with
"parallel_vacuum_*". Alternative ideas would be names starting with
"vac_*" like other functions declared in vacuum.h, or to distinguish
from them names starting with "pvac_*".

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  37. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-24T08:54:09Z

    On Wed, Nov 24, 2021 at 12:16 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Nov 24, 2021 at 1:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Nov 24, 2021 at 7:43 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Mon, Nov 22, 2021 at 1:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Fri, Nov 19, 2021 at 7:55 AM houzj.fnst@fujitsu.com
> > > > <houzj.fnst@fujitsu.com> wrote:
> > > >
> > > > >
> > > > > 4)
> > > > >
> > > > > Just a personal suggestion for the parallel related function name. Since Andres
> > > > > wanted a uniform naming pattern. Mabe we can rename the following functions:
> > > > >
> > > > > end|begin_parallel_vacuum => parallel_vacuum_end|begin
> > > > > perform_parallel_index_bulkdel|cleanup => parallel_vacuum_index_bulkdel|cleanup
> > > > >
> > > > > So that all the parallel related functions' name is like parallel_vacuum_xxx.
> > > > >
> > > >
> > > > BTW, do we really need functions
> > > > perform_parallel_index_bulkdel|cleanup? Both do some minimal
> > > > assignments and then call parallel_vacuum_all_indexes() and there is
> > > > just one caller of each. Isn't it better to just do those assignments
> > > > in the caller and directly call parallel_vacuum_all_indexes()?
> > >
> > > The reason why I declare these two functions are: (1) the fields of
> > > ParallelVacuumState are not exposed and (2) bulk-deletion and cleanup
> > > require different arguments (estimated_count is required only by
> > > cleanup).  So if we expose the fields of ParallelVacuumState, the
> > > caller can do those assignments and directly call
> > > parallel_vacuum_all_indexes(). But I'm not sure it's good if those
> > > assignments are the caller's responsibility.
> > >
> >
> > Okay, that makes sense. However, I am still not very comfortable with
> > the function naming suggested by Hou-San, do you have any thoughts on
> > that?
>
> I personally don't disagree with the names starting with
> "parallel_vacuum_*".
>

I don't have any strong opinion here but I prefer the name which makes
more sense in the context it is being used. OTOH, I see there is an
argument that it will be easier to follow and might appear consistent
if we use parallel_vacuum_*.

-- 
With Regards,
Amit Kapila.

  38. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-29T03:38:08Z

    On Wed, Nov 24, 2021 at 5:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Nov 24, 2021 at 12:16 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Nov 24, 2021 at 1:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Nov 24, 2021 at 7:43 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Mon, Nov 22, 2021 at 1:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > > On Fri, Nov 19, 2021 at 7:55 AM houzj.fnst@fujitsu.com
> > > > > <houzj.fnst@fujitsu.com> wrote:
> > > > >
> > > > > >
> > > > > > 4)
> > > > > >
> > > > > > Just a personal suggestion for the parallel related function name. Since Andres
> > > > > > wanted a uniform naming pattern. Mabe we can rename the following functions:
> > > > > >
> > > > > > end|begin_parallel_vacuum => parallel_vacuum_end|begin
> > > > > > perform_parallel_index_bulkdel|cleanup => parallel_vacuum_index_bulkdel|cleanup
> > > > > >
> > > > > > So that all the parallel related functions' name is like parallel_vacuum_xxx.
> > > > > >
> > > > >
> > > > > BTW, do we really need functions
> > > > > perform_parallel_index_bulkdel|cleanup? Both do some minimal
> > > > > assignments and then call parallel_vacuum_all_indexes() and there is
> > > > > just one caller of each. Isn't it better to just do those assignments
> > > > > in the caller and directly call parallel_vacuum_all_indexes()?
> > > >
> > > > The reason why I declare these two functions are: (1) the fields of
> > > > ParallelVacuumState are not exposed and (2) bulk-deletion and cleanup
> > > > require different arguments (estimated_count is required only by
> > > > cleanup).  So if we expose the fields of ParallelVacuumState, the
> > > > caller can do those assignments and directly call
> > > > parallel_vacuum_all_indexes(). But I'm not sure it's good if those
> > > > assignments are the caller's responsibility.
> > > >
> > >
> > > Okay, that makes sense. However, I am still not very comfortable with
> > > the function naming suggested by Hou-San, do you have any thoughts on
> > > that?
> >
> > I personally don't disagree with the names starting with
> > "parallel_vacuum_*".
> >
>
> I don't have any strong opinion here but I prefer the name which makes
> more sense in the context it is being used. OTOH, I see there is an
> argument that it will be easier to follow and might appear consistent
> if we use parallel_vacuum_*.

Maybe we can start with using parallel_vacuum_*. We can change them
later if there is an argument.

I've attached an updated patch. I don't update the terminology in
vacuum that we're discussing on another thread[1].

Regards,

[1] https://www.postgresql.org/message-id/flat/CAH2-WzktGBg4si6DEdmq3q6SoXSDqNi6MtmB8CmmTmvhsxDTLA%40mail.gmail.com

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  39. RE: parallel vacuum comments

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-11-30T05:33:09Z

    On Mon, Nov 29, 2021 11:38 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> 
> Maybe we can start with using parallel_vacuum_*. We can change them
> later if there is an argument.
> 
> I've attached an updated patch. I don't update the terminology in
> vacuum that we're discussing on another thread[1].

Hi,

I noticed the patch no longer applies on the latest source.

And few comments.
1)
+static void set_parallel_vacuum_index_status(ParallelVacuumState *pvs,
+											 bool bulkdel,
+											 int num_index_scans);
+static void parallel_vacuum_all_indexes(ParallelVacuumState *pvs, bool bulkdel,
+										int num_index_scans);
...
+static bool index_can_participate_parallel_vacuum(Relation indrel,
+												  int num_index_scans);

Maybe the parameter num_index_scans can be replaced by a bool flag since it is
only used in the check "num_index_scans > 0" and "num_index_scans == 0".

2)
+		/* Reinitialize the parallel context to relaunch parallel workers */
+		if (!pvs->first_time)

It seems the ParallelVacuumState::first_time was not initialized before ?

Best regards
Hou zj

  40. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-30T06:00:28Z

    On Tue, Nov 30, 2021 at 11:03 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Mon, Nov 29, 2021 11:38 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
>
> 2)
> +               /* Reinitialize the parallel context to relaunch parallel workers */
> +               if (!pvs->first_time)
>
> It seems the ParallelVacuumState::first_time was not initialized before ?
>

Yeah, I also notice this while looking at the patch.

One more thing it seems the patch has removed even the existing error
callback from parallel_vacuum_main. I suggested that we can enhance or
add a new one if required in a separate patch but let's keep the
current one as it is.

Can we think of splitting the patch in the following manner: (a) the
patch to get rid of bitmap to represent whether particular index
supports parallel vacuum and rename of functions (b) any other stuff
to improve the current implementation, (c) move the parallel vacuum
related code to a separate file?

I think if we can split the patch, it will be easier to review and
reduce the chances of introducing any bugs in this area.

-- 
With Regards,
Amit Kapila.

  41. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-11-30T07:45:55Z

    On Tue, Nov 30, 2021 at 3:00 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Nov 30, 2021 at 11:03 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Mon, Nov 29, 2021 11:38 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> >
> > 2)
> > +               /* Reinitialize the parallel context to relaunch parallel workers */
> > +               if (!pvs->first_time)
> >
> > It seems the ParallelVacuumState::first_time was not initialized before ?
> >
>
> Yeah, I also notice this while looking at the patch.

Thank you for the comments, Amit and Hou.

>
> One more thing it seems the patch has removed even the existing error
> callback from parallel_vacuum_main. I suggested that we can enhance or
> add a new one if required in a separate patch but let's keep the
> current one as it is.

Understood.

>
> Can we think of splitting the patch in the following manner: (a) the
> patch to get rid of bitmap to represent whether particular index
> supports parallel vacuum and rename of functions (b) any other stuff
> to improve the current implementation, (c) move the parallel vacuum
> related code to a separate file?

Okay, I'll split the patch and submit them.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  42. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-02T12:31:02Z

    On Tue, Nov 30, 2021 at 4:45 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Nov 30, 2021 at 3:00 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Nov 30, 2021 at 11:03 AM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > On Mon, Nov 29, 2021 11:38 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > >
> > > 2)
> > > +               /* Reinitialize the parallel context to relaunch parallel workers */
> > > +               if (!pvs->first_time)
> > >
> > > It seems the ParallelVacuumState::first_time was not initialized before ?
> > >
> >
> > Yeah, I also notice this while looking at the patch.
>
> Thank you for the comments, Amit and Hou.
>
> >
> > One more thing it seems the patch has removed even the existing error
> > callback from parallel_vacuum_main. I suggested that we can enhance or
> > add a new one if required in a separate patch but let's keep the
> > current one as it is.
>
> Understood.
>
> >
> > Can we think of splitting the patch in the following manner: (a) the
> > patch to get rid of bitmap to represent whether particular index
> > supports parallel vacuum and rename of functions (b) any other stuff
> > to improve the current implementation, (c) move the parallel vacuum
> > related code to a separate file?
>
> Okay, I'll split the patch and submit them.
>

I've attached updated patches.

The first patch is the main patch for refactoring parallel vacuum
code; removes bitmap-related code and renames function names for
consistency. The second patch moves these parallel-related codes to
vacuumparallel.c as well as common functions that are used by both
lazyvacuum.c and vacuumparallel.c to vacuum.c. The third patch adds
regression tests for parallel vacuum on different kinds of indexes
with multiple index scans. Please review them.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  43. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-03T09:03:13Z

    On Thu, Dec 2, 2021 at 6:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've attached updated patches.
>

I have a few comments on v4-0001.
1.
In parallel_vacuum_process_all_indexes(), we can combine the two
checks for vacuum/cleanup at the beginning of the function and I think
it is better to keep the variable name as bulkdel or cleanup instead
of vacuum as that is more specific and clear.

2. The patch seems to be calling parallel_vacuum_should_skip_index
thrice even before starting parallel vacuum. It has a call to find the
number of blocks which has to be performed for each index. I
understand it might not be too costly to call this but it seems better
to remember this info like we are doing in the current code. We can
probably set parallel_workers_can_process in parallel_vacuum_begin and
then again update in parallel_vacuum_process_all_indexes. Won't doing
something like that be better?

3.  /*
  * Copy the index bulk-deletion result returned from ambulkdelete and
@@ -2940,19 +2935,20 @@ parallel_process_one_index(Relation indrel,
  * Since all vacuum workers write the bulk-deletion result at different
  * slots we can write them without locking.
  */
- if (shared_istat && !shared_istat->updated && istat_res != NULL)
+ if (!pindstats->istat_updated && istat_res != NULL)
  {
- memcpy(&shared_istat->istat, istat_res, sizeof(IndexBulkDeleteResult));
- shared_istat->updated = true;
+ memcpy(&(pindstats->istat), istat_res, sizeof(IndexBulkDeleteResult));
+ pindstats->istat_updated = true;

  /* Free the locally-allocated bulk-deletion result */
  pfree(istat_res);
-
- /* return the pointer to the result from shared memory */
- return &shared_istat->istat;
  }

I think here now we copy the results both for local and parallel
cleanup. Isn't it better to write something about that in comments as
it is not clear from current comments?

4.
+ /* Estimate size for shared information -- PARALLEL_VACUUM_KEY_SHARED */
+ est_shared_len = MAXALIGN(sizeof(LVShared));
  shm_toc_estimate_chunk(&pcxt->estimator, est_shared_len);

Do we need MAXALIGN here? I think previously we required it here
because immediately after this we were writing index stats but now
those are allocated separately. Normally, shm_toc_estimate_chunk()
aligns the size but sometimes we need to do it so as to avoid
unaligned accessing of shared mem. I am really not sure whether we
require it for dead_items, do you remember why it is there in the
first place.

5. The below-updated comment based on one of my previous suggestions
seems to be missing in this version.
+ /*
+ * Not safe, if the index supports parallel cleanup conditionally,
+ * but we have already processed the index (for bulkdelete).  We do
+ * this to avoid the need to invoke workers when parallel index
+ * cleanup doesn't need to scan the index.  See the comments for
+ * option VACUUM_OPTION_PARALLEL_COND_CLEANUP to know when indexes
+ * support parallel cleanup conditionally.
+ */

-- 
With Regards,
Amit Kapila.

  44. RE: parallel vacuum comments

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-12-03T09:31:38Z

    On Thur, Dec 2, 2021 8:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've attached updated patches.
> 
> The first patch is the main patch for refactoring parallel vacuum code; removes
> bitmap-related code and renames function names for consistency. The second
> patch moves these parallel-related codes to vacuumparallel.c as well as
> common functions that are used by both lazyvacuum.c and vacuumparallel.c to
> vacuum.c. The third patch adds regression tests for parallel vacuum on
> different kinds of indexes with multiple index scans. Please review them.

Thanks for updating the patch.
I reviewed the 0001 patch and didn’t find some big issues in the patch.

I only have a personally suggestion for the following function name:

parallel_vacuum_process_unsafe_indexes
parallel_vacuum_index_is_parallel_safe

It seems not only "unsafe" index are processed in the above functions,
but also index which is unsuitable(based on parallel_vacuum_should_skip_index).
So, it might be clear to avoid "unsafe" in the name. Maybe we can use: "xxin_leader"
or " can_participate".

Best regards,
Hou zj

  45. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-03T09:36:31Z

    On Fri, Dec 3, 2021 at 3:01 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Thur, Dec 2, 2021 8:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > I've attached updated patches.
> >
> > The first patch is the main patch for refactoring parallel vacuum code; removes
> > bitmap-related code and renames function names for consistency. The second
> > patch moves these parallel-related codes to vacuumparallel.c as well as
> > common functions that are used by both lazyvacuum.c and vacuumparallel.c to
> > vacuum.c. The third patch adds regression tests for parallel vacuum on
> > different kinds of indexes with multiple index scans. Please review them.
>
> Thanks for updating the patch.
> I reviewed the 0001 patch and didn’t find some big issues in the patch.
>
> I only have a personally suggestion for the following function name:
>
> parallel_vacuum_process_unsafe_indexes
> parallel_vacuum_index_is_parallel_safe
>
> It seems not only "unsafe" index are processed in the above functions,
> but also index which is unsuitable(based on parallel_vacuum_should_skip_index).
>

I have given one comment to remove the call to
parallel_vacuum_should_skip_index() from
parallel_vacuum_index_is_parallel_safe(). If Sawada-San follows that
then maybe your point will be addressed.

-- 
With Regards,
Amit Kapila.

  46. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-03T09:56:46Z

    On Fri, Dec 3, 2021 at 2:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Dec 2, 2021 at 6:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached updated patches.
> >
>
> I have a few comments on v4-0001.
>

The new test proposed by v4-0003 is increasing the vacuum_parallel.sql
timing by more than 10 times. It appears to be taking the highest time
among all the tests in make check. I think it is because of a large
amount of data being processed by the test. I think it is good to use
it for testing of patches during development but won't be a good idea
to commit. Can we reduce its timings?

-- 
With Regards,
Amit Kapila.

  47. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-03T12:33:14Z

     and

On Fri, Dec 3, 2021 at 6:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Dec 3, 2021 at 2:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Dec 2, 2021 at 6:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > I've attached updated patches.
> > >
> >
> > I have a few comments on v4-0001.
> >
>
> The new test proposed by v4-0003 is increasing the vacuum_parallel.sql
> timing by more than 10 times. It appears to be taking the highest time
> among all the tests in make check. I think it is because of a large
> amount of data being processed by the test.

Right.

> I think it is good to use
> it for testing of patches during development but won't be a good idea
> to commit. Can we reduce its timings?

On reflection, we already have test cases for:

* a parallel vacuum does bulkdeletion and cleanup
* a parallel vacuum does only cleanup

and the case that the new tests add is that a vacuum does bulkdeletion
twice and cleanup. Given the increasing the regression test time, the
thrid patch might not worth to be added.


Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  48. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-03T12:36:18Z

    On Fri, Dec 3, 2021 at 6:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Dec 2, 2021 at 6:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached updated patches.
> >
>
> I have a few comments on v4-0001.

Thank you for the comments!

> 1.
> In parallel_vacuum_process_all_indexes(), we can combine the two
> checks for vacuum/cleanup at the beginning of the function

Agreed.

> and I think
> it is better to keep the variable name as bulkdel or cleanup instead
> of vacuum as that is more specific and clear.

I was thinking to use the terms "bulkdel" and "cleanup" instead of
"vacuum" and "cleanup" for the same reason. That way, probably we can
use “bulkdel" and “cleanup" when doing index bulk-deletion (i.g.,
calling to ambulkdelete) and index cleanup (calling to
amvacuumcleanup), respectively, and use "vacuum" when processing an
index, i.g., doing either bulk-delete or cleanup, instead of using
just "processing" . But we already use “vacuum” and “cleanup” in many
places, e.g., lazy_vacuum_index() and lazy_cleanup_index(). If we want
to use “bulkdel” instead of “vacuum”, I think it would be better to
change the terminology in vacuumlazy.c thoroughly, probably in a
separate patch.

> 2. The patch seems to be calling parallel_vacuum_should_skip_index
> thrice even before starting parallel vacuum. It has a call to find the
> number of blocks which has to be performed for each index. I
> understand it might not be too costly to call this but it seems better
> to remember this info like we are doing in the current code.

Yes, we can bring will_vacuum_parallel array back to the code. That
way, we can remove the call to parallel_vacuum_should_skip_index() in
parallel_vacuum_begin().

> We can
> probably set parallel_workers_can_process in parallel_vacuum_begin and
> then again update in parallel_vacuum_process_all_indexes. Won't doing
> something like that be better?

parallel_workers_can_process can vary depending on bulk-deletion, the
first time cleanup, or the second time (or more) cleanup. What can we
set parallel_workers_can_process based on in parallel_vacuum_begin()?

>
> 3.  /*
>   * Copy the index bulk-deletion result returned from ambulkdelete and
> @@ -2940,19 +2935,20 @@ parallel_process_one_index(Relation indrel,
>   * Since all vacuum workers write the bulk-deletion result at different
>   * slots we can write them without locking.
>   */
> - if (shared_istat && !shared_istat->updated && istat_res != NULL)
> + if (!pindstats->istat_updated && istat_res != NULL)
>   {
> - memcpy(&shared_istat->istat, istat_res, sizeof(IndexBulkDeleteResult));
> - shared_istat->updated = true;
> + memcpy(&(pindstats->istat), istat_res, sizeof(IndexBulkDeleteResult));
> + pindstats->istat_updated = true;
>
>   /* Free the locally-allocated bulk-deletion result */
>   pfree(istat_res);
> -
> - /* return the pointer to the result from shared memory */
> - return &shared_istat->istat;
>   }
>
> I think here now we copy the results both for local and parallel
> cleanup. Isn't it better to write something about that in comments as
> it is not clear from current comments?

What do you mean by "local cleanup"?

>
> 4.
> + /* Estimate size for shared information -- PARALLEL_VACUUM_KEY_SHARED */
> + est_shared_len = MAXALIGN(sizeof(LVShared));
>   shm_toc_estimate_chunk(&pcxt->estimator, est_shared_len);
>
> Do we need MAXALIGN here? I think previously we required it here
> because immediately after this we were writing index stats but now
> those are allocated separately. Normally, shm_toc_estimate_chunk()
> aligns the size but sometimes we need to do it so as to avoid
> unaligned accessing of shared mem. I am really not sure whether we
> require it for dead_items, do you remember why it is there in the
> first place.

Indeed. I don't remember that. Probably it's an oversight.

>
> 5. The below-updated comment based on one of my previous suggestions
> seems to be missing in this version.
> + /*
> + * Not safe, if the index supports parallel cleanup conditionally,
> + * but we have already processed the index (for bulkdelete).  We do
> + * this to avoid the need to invoke workers when parallel index
> + * cleanup doesn't need to scan the index.  See the comments for
> + * option VACUUM_OPTION_PARALLEL_COND_CLEANUP to know when indexes
> + * support parallel cleanup conditionally.
> + */

Added.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  49. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-06T04:47:06Z

    On Fri, Dec 3, 2021 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Dec 3, 2021 at 6:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Dec 2, 2021 at 6:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > I've attached updated patches.
> > >
> >
> > I have a few comments on v4-0001.
>
> Thank you for the comments!
>
> > 1.
> > In parallel_vacuum_process_all_indexes(), we can combine the two
> > checks for vacuum/cleanup at the beginning of the function
>
> Agreed.
>
> > and I think
> > it is better to keep the variable name as bulkdel or cleanup instead
> > of vacuum as that is more specific and clear.
>
> I was thinking to use the terms "bulkdel" and "cleanup" instead of
> "vacuum" and "cleanup" for the same reason. That way, probably we can
> use “bulkdel" and “cleanup" when doing index bulk-deletion (i.g.,
> calling to ambulkdelete) and index cleanup (calling to
> amvacuumcleanup), respectively, and use "vacuum" when processing an
> index, i.g., doing either bulk-delete or cleanup, instead of using
> just "processing" . But we already use “vacuum” and “cleanup” in many
> places, e.g., lazy_vacuum_index() and lazy_cleanup_index(). If we want
> to use “bulkdel” instead of “vacuum”, I think it would be better to
> change the terminology in vacuumlazy.c thoroughly, probably in a
> separate patch.
>

Okay.

> > 2. The patch seems to be calling parallel_vacuum_should_skip_index
> > thrice even before starting parallel vacuum. It has a call to find the
> > number of blocks which has to be performed for each index. I
> > understand it might not be too costly to call this but it seems better
> > to remember this info like we are doing in the current code.
>
> Yes, we can bring will_vacuum_parallel array back to the code. That
> way, we can remove the call to parallel_vacuum_should_skip_index() in
> parallel_vacuum_begin().
>
> > We can
> > probably set parallel_workers_can_process in parallel_vacuum_begin and
> > then again update in parallel_vacuum_process_all_indexes. Won't doing
> > something like that be better?
>
> parallel_workers_can_process can vary depending on bulk-deletion, the
> first time cleanup, or the second time (or more) cleanup. What can we
> set parallel_workers_can_process based on in parallel_vacuum_begin()?
>

I was thinking to set the results of will_vacuum_parallel in
parallel_vacuum_begin().

> >
> > 3.  /*
> >   * Copy the index bulk-deletion result returned from ambulkdelete and
> > @@ -2940,19 +2935,20 @@ parallel_process_one_index(Relation indrel,
> >   * Since all vacuum workers write the bulk-deletion result at different
> >   * slots we can write them without locking.
> >   */
> > - if (shared_istat && !shared_istat->updated && istat_res != NULL)
> > + if (!pindstats->istat_updated && istat_res != NULL)
> >   {
> > - memcpy(&shared_istat->istat, istat_res, sizeof(IndexBulkDeleteResult));
> > - shared_istat->updated = true;
> > + memcpy(&(pindstats->istat), istat_res, sizeof(IndexBulkDeleteResult));
> > + pindstats->istat_updated = true;
> >
> >   /* Free the locally-allocated bulk-deletion result */
> >   pfree(istat_res);
> > -
> > - /* return the pointer to the result from shared memory */
> > - return &shared_istat->istat;
> >   }
> >
> > I think here now we copy the results both for local and parallel
> > cleanup. Isn't it better to write something about that in comments as
> > it is not clear from current comments?
>
> What do you mean by "local cleanup"?
>

Clean-up performed by leader backend.

> >
> > 4.
> > + /* Estimate size for shared information -- PARALLEL_VACUUM_KEY_SHARED */
> > + est_shared_len = MAXALIGN(sizeof(LVShared));
> >   shm_toc_estimate_chunk(&pcxt->estimator, est_shared_len);
> >
> > Do we need MAXALIGN here? I think previously we required it here
> > because immediately after this we were writing index stats but now
> > those are allocated separately. Normally, shm_toc_estimate_chunk()
> > aligns the size but sometimes we need to do it so as to avoid
> > unaligned accessing of shared mem. I am really not sure whether we
> > require it for dead_items, do you remember why it is there in the
> > first place.
>
> Indeed. I don't remember that. Probably it's an oversight.
>

Yeah, I also think so.

-- 
With Regards,
Amit Kapila.

  50. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-07T01:24:14Z

    On Mon, Dec 6, 2021 at 1:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Dec 3, 2021 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Dec 3, 2021 at 6:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Dec 2, 2021 at 6:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > I've attached updated patches.
> > > >
> > >
> > > I have a few comments on v4-0001.
> >
> > Thank you for the comments!
> >
> > > 1.
> > > In parallel_vacuum_process_all_indexes(), we can combine the two
> > > checks for vacuum/cleanup at the beginning of the function
> >
> > Agreed.
> >
> > > and I think
> > > it is better to keep the variable name as bulkdel or cleanup instead
> > > of vacuum as that is more specific and clear.
> >
> > I was thinking to use the terms "bulkdel" and "cleanup" instead of
> > "vacuum" and "cleanup" for the same reason. That way, probably we can
> > use “bulkdel" and “cleanup" when doing index bulk-deletion (i.g.,
> > calling to ambulkdelete) and index cleanup (calling to
> > amvacuumcleanup), respectively, and use "vacuum" when processing an
> > index, i.g., doing either bulk-delete or cleanup, instead of using
> > just "processing" . But we already use “vacuum” and “cleanup” in many
> > places, e.g., lazy_vacuum_index() and lazy_cleanup_index(). If we want
> > to use “bulkdel” instead of “vacuum”, I think it would be better to
> > change the terminology in vacuumlazy.c thoroughly, probably in a
> > separate patch.
> >
>
> Okay.
>
> > > 2. The patch seems to be calling parallel_vacuum_should_skip_index
> > > thrice even before starting parallel vacuum. It has a call to find the
> > > number of blocks which has to be performed for each index. I
> > > understand it might not be too costly to call this but it seems better
> > > to remember this info like we are doing in the current code.
> >
> > Yes, we can bring will_vacuum_parallel array back to the code. That
> > way, we can remove the call to parallel_vacuum_should_skip_index() in
> > parallel_vacuum_begin().
> >
> > > We can
> > > probably set parallel_workers_can_process in parallel_vacuum_begin and
> > > then again update in parallel_vacuum_process_all_indexes. Won't doing
> > > something like that be better?
> >
> > parallel_workers_can_process can vary depending on bulk-deletion, the
> > first time cleanup, or the second time (or more) cleanup. What can we
> > set parallel_workers_can_process based on in parallel_vacuum_begin()?
> >
>
> I was thinking to set the results of will_vacuum_parallel in
> parallel_vacuum_begin().
>
> > >
> > > 3.  /*
> > >   * Copy the index bulk-deletion result returned from ambulkdelete and
> > > @@ -2940,19 +2935,20 @@ parallel_process_one_index(Relation indrel,
> > >   * Since all vacuum workers write the bulk-deletion result at different
> > >   * slots we can write them without locking.
> > >   */
> > > - if (shared_istat && !shared_istat->updated && istat_res != NULL)
> > > + if (!pindstats->istat_updated && istat_res != NULL)
> > >   {
> > > - memcpy(&shared_istat->istat, istat_res, sizeof(IndexBulkDeleteResult));
> > > - shared_istat->updated = true;
> > > + memcpy(&(pindstats->istat), istat_res, sizeof(IndexBulkDeleteResult));
> > > + pindstats->istat_updated = true;
> > >
> > >   /* Free the locally-allocated bulk-deletion result */
> > >   pfree(istat_res);
> > > -
> > > - /* return the pointer to the result from shared memory */
> > > - return &shared_istat->istat;
> > >   }
> > >
> > > I think here now we copy the results both for local and parallel
> > > cleanup. Isn't it better to write something about that in comments as
> > > it is not clear from current comments?
> >
> > What do you mean by "local cleanup"?
> >
>
> Clean-up performed by leader backend.

I might be missing your points but I think the patch doesn't change
the behavior around these codes. With the patch, we allocate
IndexBulkDeleteResult on DSM for every index but the patch doesn't
change the fact that in parallel vacuum/cleanup cases, we copy
IndexBulkDeleteResult returned from ambulkdelete() or amvacuumcleanup
to DSM space. Non-parallel vacuum doesn't use this function. Do you
have any suggestions on better comments here?

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  51. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-07T03:23:27Z

    On Tue, Dec 7, 2021 at 6:54 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Dec 6, 2021 at 1:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Dec 3, 2021 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > >
> > > > 3.  /*
> > > >   * Copy the index bulk-deletion result returned from ambulkdelete and
> > > > @@ -2940,19 +2935,20 @@ parallel_process_one_index(Relation indrel,
> > > >   * Since all vacuum workers write the bulk-deletion result at different
> > > >   * slots we can write them without locking.
> > > >   */
> > > > - if (shared_istat && !shared_istat->updated && istat_res != NULL)
> > > > + if (!pindstats->istat_updated && istat_res != NULL)
> > > >   {
> > > > - memcpy(&shared_istat->istat, istat_res, sizeof(IndexBulkDeleteResult));
> > > > - shared_istat->updated = true;
> > > > + memcpy(&(pindstats->istat), istat_res, sizeof(IndexBulkDeleteResult));
> > > > + pindstats->istat_updated = true;
> > > >
> > > >   /* Free the locally-allocated bulk-deletion result */
> > > >   pfree(istat_res);
> > > > -
> > > > - /* return the pointer to the result from shared memory */
> > > > - return &shared_istat->istat;
> > > >   }
> > > >
> > > > I think here now we copy the results both for local and parallel
> > > > cleanup. Isn't it better to write something about that in comments as
> > > > it is not clear from current comments?
> > >
> > > What do you mean by "local cleanup"?
> > >
> >
> > Clean-up performed by leader backend.
>
> I might be missing your points but I think the patch doesn't change
> the behavior around these codes. With the patch, we allocate
> IndexBulkDeleteResult on DSM for every index but the patch doesn't
> change the fact that in parallel vacuum/cleanup cases, we copy
> IndexBulkDeleteResult returned from ambulkdelete() or amvacuumcleanup
> to DSM space. Non-parallel vacuum doesn't use this function.
>

I was talking about when we call parallel_vacuum_process_one_index()
via parallel_vacuum_process_unsafe_indexes() where the leader
processes the indexes that will be skipped by workers. Isn't that case
slightly different now because previously in that case we would not
have done the copy but now we will copy the stats in that case as
well? Am, I missing something?

-- 
With Regards,
Amit Kapila.

  52. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-07T05:42:27Z

    On Tue, Dec 7, 2021 at 12:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Dec 7, 2021 at 6:54 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Dec 6, 2021 at 1:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, Dec 3, 2021 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > >
> > > > > 3.  /*
> > > > >   * Copy the index bulk-deletion result returned from ambulkdelete and
> > > > > @@ -2940,19 +2935,20 @@ parallel_process_one_index(Relation indrel,
> > > > >   * Since all vacuum workers write the bulk-deletion result at different
> > > > >   * slots we can write them without locking.
> > > > >   */
> > > > > - if (shared_istat && !shared_istat->updated && istat_res != NULL)
> > > > > + if (!pindstats->istat_updated && istat_res != NULL)
> > > > >   {
> > > > > - memcpy(&shared_istat->istat, istat_res, sizeof(IndexBulkDeleteResult));
> > > > > - shared_istat->updated = true;
> > > > > + memcpy(&(pindstats->istat), istat_res, sizeof(IndexBulkDeleteResult));
> > > > > + pindstats->istat_updated = true;
> > > > >
> > > > >   /* Free the locally-allocated bulk-deletion result */
> > > > >   pfree(istat_res);
> > > > > -
> > > > > - /* return the pointer to the result from shared memory */
> > > > > - return &shared_istat->istat;
> > > > >   }
> > > > >
> > > > > I think here now we copy the results both for local and parallel
> > > > > cleanup. Isn't it better to write something about that in comments as
> > > > > it is not clear from current comments?
> > > >
> > > > What do you mean by "local cleanup"?
> > > >
> > >
> > > Clean-up performed by leader backend.
> >
> > I might be missing your points but I think the patch doesn't change
> > the behavior around these codes. With the patch, we allocate
> > IndexBulkDeleteResult on DSM for every index but the patch doesn't
> > change the fact that in parallel vacuum/cleanup cases, we copy
> > IndexBulkDeleteResult returned from ambulkdelete() or amvacuumcleanup
> > to DSM space. Non-parallel vacuum doesn't use this function.
> >
>
> I was talking about when we call parallel_vacuum_process_one_index()
> via parallel_vacuum_process_unsafe_indexes() where the leader
> processes the indexes that will be skipped by workers. Isn't that case
> slightly different now because previously in that case we would not
> have done the copy but now we will copy the stats in that case as
> well? Am, I missing something?
>

I got your point. Yes, with the patch, we copy the stats to DSM even
if the index doesn't participate in parallel vacuum at all.
Previously, these statistics are allocated in the local memory.
Updated the comments at the declaration of lvpindstats.

I've attached an updated patch. I've removed 0003 patch that added
regression tests as per discussion. Regarding the terminology like
"bulkdel" and "cleanup" you pointed out, I've done that in 0002 patch
while moving the code to vacuumparallel.c. In that file, we can
consistently use the terms "bulkdel" and "cleanup" instead of "vacuum"
and "cleanup".

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  53. RE: parallel vacuum comments

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-12-08T03:22:16Z

    On Tuesday, December 7, 2021 1:42 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've attached an updated patch. I've removed 0003 patch that added
> regression tests as per discussion. Regarding the terminology like "bulkdel"
> and "cleanup" you pointed out, I've done that in 0002 patch while moving the
> code to vacuumparallel.c. In that file, we can consistently use the terms
> "bulkdel" and "cleanup" instead of "vacuum"
> and "cleanup".
Hi,

Thanks for updating the patch.
I noticed few minor things.

0001
1)

 		 * Skip processing indexes that are unsafe for workers (these are
-		 * processed in do_serial_processing_for_unsafe_indexes() by leader)
+		 * processed in parallel_vacuum_process_unsafe_indexes() by leader)

It might be clearer to mention that the index to be skipped are unsafe OR not
worthwhile.

2)
+	/* Set index vacuum status and mark as parallel safe or not */
+	for (int i = 0; i < pvc->nindexes; i++)
+	{
	...
+		pindstats->parallel_workers_can_process =
+			parallel_vacuum_index_is_parallel_safe(vacrel,
+												   vacrel->indrels[i],
+												   vacuum);

For the comments above the loop, maybe better to mention we are marking whether
worker can process the index(not only safe/unsafe).

0002
3)

+		/*
+		 * Skip indexes that are unsuitable target for parallel index vacuum
+		 */
+		if (parallel_vacuum_should_skip_index(indrel))
+			continue;
+

It seems we can use will_parallel_vacuum[] here instead of invoking the function
again.

Best regards,
Hou zj

  54. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-08T04:37:57Z

    On Wed, Dec 8, 2021 at 12:22 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Tuesday, December 7, 2021 1:42 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > I've attached an updated patch. I've removed 0003 patch that added
> > regression tests as per discussion. Regarding the terminology like "bulkdel"
> > and "cleanup" you pointed out, I've done that in 0002 patch while moving the
> > code to vacuumparallel.c. In that file, we can consistently use the terms
> > "bulkdel" and "cleanup" instead of "vacuum"
> > and "cleanup".
> Hi,
>
> Thanks for updating the patch.
> I noticed few minor things.

Thank you for the comments!

>
> 0001
> 1)
>
>                  * Skip processing indexes that are unsafe for workers (these are
> -                * processed in do_serial_processing_for_unsafe_indexes() by leader)
> +                * processed in parallel_vacuum_process_unsafe_indexes() by leader)
>
> It might be clearer to mention that the index to be skipped are unsafe OR not
> worthwhile.

Agreed. Will add the comments.

>
> 2)
> +       /* Set index vacuum status and mark as parallel safe or not */
> +       for (int i = 0; i < pvc->nindexes; i++)
> +       {
>         ...
> +               pindstats->parallel_workers_can_process =
> +                       parallel_vacuum_index_is_parallel_safe(vacrel,
> +                                                                                                  vacrel->indrels[i],
> +                                                                                                  vacuum);
>
> For the comments above the loop, maybe better to mention we are marking whether
> worker can process the index(not only safe/unsafe).

Right. WIll fix.

>
> 0002
> 3)
>
> +               /*
> +                * Skip indexes that are unsuitable target for parallel index vacuum
> +                */
> +               if (parallel_vacuum_should_skip_index(indrel))
> +                       continue;
> +
>
> It seems we can use will_parallel_vacuum[] here instead of invoking the function
> again.

Oops, I missed updating it in 0002 patch. Will fix.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  55. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-08T23:48:20Z

    On Wed, Dec 8, 2021 at 1:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Dec 8, 2021 at 12:22 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Tuesday, December 7, 2021 1:42 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > I've attached an updated patch. I've removed 0003 patch that added
> > > regression tests as per discussion. Regarding the terminology like "bulkdel"
> > > and "cleanup" you pointed out, I've done that in 0002 patch while moving the
> > > code to vacuumparallel.c. In that file, we can consistently use the terms
> > > "bulkdel" and "cleanup" instead of "vacuum"
> > > and "cleanup".
> > Hi,
> >
> > Thanks for updating the patch.
> > I noticed few minor things.
>
> Thank you for the comments!
>
> >
> > 0001
> > 1)
> >
> >                  * Skip processing indexes that are unsafe for workers (these are
> > -                * processed in do_serial_processing_for_unsafe_indexes() by leader)
> > +                * processed in parallel_vacuum_process_unsafe_indexes() by leader)
> >
> > It might be clearer to mention that the index to be skipped are unsafe OR not
> > worthwhile.
>
> Agreed. Will add the comments.
>
> >
> > 2)
> > +       /* Set index vacuum status and mark as parallel safe or not */
> > +       for (int i = 0; i < pvc->nindexes; i++)
> > +       {
> >         ...
> > +               pindstats->parallel_workers_can_process =
> > +                       parallel_vacuum_index_is_parallel_safe(vacrel,
> > +                                                                                                  vacrel->indrels[i],
> > +                                                                                                  vacuum);
> >
> > For the comments above the loop, maybe better to mention we are marking whether
> > worker can process the index(not only safe/unsafe).
>
> Right. WIll fix.
>
> >
> > 0002
> > 3)
> >
> > +               /*
> > +                * Skip indexes that are unsuitable target for parallel index vacuum
> > +                */
> > +               if (parallel_vacuum_should_skip_index(indrel))
> > +                       continue;
> > +
> >
> > It seems we can use will_parallel_vacuum[] here instead of invoking the function
> > again.
>
> Oops, I missed updating it in 0002 patch. Will fix.

I've attached updated patches. Please review them.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  56. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-09T10:05:38Z

    On Mon, Dec 6, 2021 at 10:17 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Dec 3, 2021 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Dec 3, 2021 at 6:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Dec 2, 2021 at 6:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > I've attached updated patches.
> > > >
> > >
> > > I have a few comments on v4-0001.
> >
> > Thank you for the comments!
> >
> > > 1.
> > > In parallel_vacuum_process_all_indexes(), we can combine the two
> > > checks for vacuum/cleanup at the beginning of the function
> >
> > Agreed.
> >
> > > and I think
> > > it is better to keep the variable name as bulkdel or cleanup instead
> > > of vacuum as that is more specific and clear.
> >
> > I was thinking to use the terms "bulkdel" and "cleanup" instead of
> > "vacuum" and "cleanup" for the same reason. That way, probably we can
> > use “bulkdel" and “cleanup" when doing index bulk-deletion (i.g.,
> > calling to ambulkdelete) and index cleanup (calling to
> > amvacuumcleanup), respectively, and use "vacuum" when processing an
> > index, i.g., doing either bulk-delete or cleanup, instead of using
> > just "processing" . But we already use “vacuum” and “cleanup” in many
> > places, e.g., lazy_vacuum_index() and lazy_cleanup_index(). If we want
> > to use “bulkdel” instead of “vacuum”, I think it would be better to
> > change the terminology in vacuumlazy.c thoroughly, probably in a
> > separate patch.
> >
>
> Okay.
>
> > > 2. The patch seems to be calling parallel_vacuum_should_skip_index
> > > thrice even before starting parallel vacuum. It has a call to find the
> > > number of blocks which has to be performed for each index. I
> > > understand it might not be too costly to call this but it seems better
> > > to remember this info like we are doing in the current code.
> >
> > Yes, we can bring will_vacuum_parallel array back to the code. That
> > way, we can remove the call to parallel_vacuum_should_skip_index() in
> > parallel_vacuum_begin().
> >
> > > We can
> > > probably set parallel_workers_can_process in parallel_vacuum_begin and
> > > then again update in parallel_vacuum_process_all_indexes. Won't doing
> > > something like that be better?
> >
> > parallel_workers_can_process can vary depending on bulk-deletion, the
> > first time cleanup, or the second time (or more) cleanup. What can we
> > set parallel_workers_can_process based on in parallel_vacuum_begin()?
> >
>
> I was thinking to set the results of will_vacuum_parallel in
> parallel_vacuum_begin().
>

This point doesn't seem to be addressed in the latest version (v6). Is
there a reason for not doing it? If we do this, then we don't need to
call parallel_vacuum_should_skip_index() from
parallel_vacuum_index_is_parallel_safe().

-- 
With Regards,
Amit Kapila.

  57. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-09T10:44:35Z

    On Thu, Dec 9, 2021 at 3:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Dec 6, 2021 at 10:17 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Dec 3, 2021 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > 2. The patch seems to be calling parallel_vacuum_should_skip_index
> > > > thrice even before starting parallel vacuum. It has a call to find the
> > > > number of blocks which has to be performed for each index. I
> > > > understand it might not be too costly to call this but it seems better
> > > > to remember this info like we are doing in the current code.
> > >
> > > Yes, we can bring will_vacuum_parallel array back to the code. That
> > > way, we can remove the call to parallel_vacuum_should_skip_index() in
> > > parallel_vacuum_begin().
> > >
> > > > We can
> > > > probably set parallel_workers_can_process in parallel_vacuum_begin and
> > > > then again update in parallel_vacuum_process_all_indexes. Won't doing
> > > > something like that be better?
> > >
> > > parallel_workers_can_process can vary depending on bulk-deletion, the
> > > first time cleanup, or the second time (or more) cleanup. What can we
> > > set parallel_workers_can_process based on in parallel_vacuum_begin()?
> > >
> >
> > I was thinking to set the results of will_vacuum_parallel in
> > parallel_vacuum_begin().
> >
>
> This point doesn't seem to be addressed in the latest version (v6). Is
> there a reason for not doing it? If we do this, then we don't need to
> call parallel_vacuum_should_skip_index() from
> parallel_vacuum_index_is_parallel_safe().
>

Few minor comments on v6-0001
==========================
1.
The array
+ * element is allocated for every index, even those indexes where
+ * parallel index vacuuming is unsafe or not worthwhile (i.g.,
+ * parallel_vacuum_should_skip_index() returns true).

/i.g/e.g

2.
static void update_index_statistics(LVRelState *vacrel);
-static void begin_parallel_vacuum(LVRelState *vacrel, int nrequested);
-static void end_parallel_vacuum(LVRelState *vacrel);
-static LVSharedIndStats *parallel_stats_for_idx(LVShared *lvshared,
int getidx);
-static bool parallel_processing_is_safe(Relation indrel, LVShared *lvshared);
+
+static int parallel_vacuum_compute_workers(LVRelState *vacrel, int nrequested,
+    bool *will_parallel_vacuum);

In declaration, parallel_vacuum_compute_workers() is declared after
update_index_statistics but later defined in reverse order. I suggest
to make the order of definitions same as their declaration. Similarly,
the order of definition of parallel_vacuum_process_all_indexes(),
parallel_vacuum_process_unsafe_indexes(),
parallel_vacuum_process_safe_indexes(),
parallel_vacuum_process_one_index() doesn't match the order of their
declaration. Can we change that as well?

-- 
With Regards,
Amit Kapila.

  58. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-09T12:22:30Z

    On Thu, Dec 9, 2021 at 7:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Dec 6, 2021 at 10:17 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Dec 3, 2021 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Fri, Dec 3, 2021 at 6:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Thu, Dec 2, 2021 at 6:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > >
> > > > > I've attached updated patches.
> > > > >
> > > >
> > > > I have a few comments on v4-0001.
> > >
> > > Thank you for the comments!
> > >
> > > > 1.
> > > > In parallel_vacuum_process_all_indexes(), we can combine the two
> > > > checks for vacuum/cleanup at the beginning of the function
> > >
> > > Agreed.
> > >
> > > > and I think
> > > > it is better to keep the variable name as bulkdel or cleanup instead
> > > > of vacuum as that is more specific and clear.
> > >
> > > I was thinking to use the terms "bulkdel" and "cleanup" instead of
> > > "vacuum" and "cleanup" for the same reason. That way, probably we can
> > > use “bulkdel" and “cleanup" when doing index bulk-deletion (i.g.,
> > > calling to ambulkdelete) and index cleanup (calling to
> > > amvacuumcleanup), respectively, and use "vacuum" when processing an
> > > index, i.g., doing either bulk-delete or cleanup, instead of using
> > > just "processing" . But we already use “vacuum” and “cleanup” in many
> > > places, e.g., lazy_vacuum_index() and lazy_cleanup_index(). If we want
> > > to use “bulkdel” instead of “vacuum”, I think it would be better to
> > > change the terminology in vacuumlazy.c thoroughly, probably in a
> > > separate patch.
> > >
> >
> > Okay.
> >
> > > > 2. The patch seems to be calling parallel_vacuum_should_skip_index
> > > > thrice even before starting parallel vacuum. It has a call to find the
> > > > number of blocks which has to be performed for each index. I
> > > > understand it might not be too costly to call this but it seems better
> > > > to remember this info like we are doing in the current code.
> > >
> > > Yes, we can bring will_vacuum_parallel array back to the code. That
> > > way, we can remove the call to parallel_vacuum_should_skip_index() in
> > > parallel_vacuum_begin().
> > >
> > > > We can
> > > > probably set parallel_workers_can_process in parallel_vacuum_begin and
> > > > then again update in parallel_vacuum_process_all_indexes. Won't doing
> > > > something like that be better?
> > >
> > > parallel_workers_can_process can vary depending on bulk-deletion, the
> > > first time cleanup, or the second time (or more) cleanup. What can we
> > > set parallel_workers_can_process based on in parallel_vacuum_begin()?
> > >
> >
> > I was thinking to set the results of will_vacuum_parallel in
> > parallel_vacuum_begin().
> >
>
> This point doesn't seem to be addressed in the latest version (v6). Is
> there a reason for not doing it? If we do this, then we don't need to
> call parallel_vacuum_should_skip_index() from
> parallel_vacuum_index_is_parallel_safe().

Probably I had misunderstood your point. I'll fix it in the next
version patch and send it soon.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  59. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-09T12:34:36Z

    On Thu, Dec 9, 2021 at 7:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Dec 9, 2021 at 3:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Dec 6, 2021 at 10:17 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, Dec 3, 2021 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > > 2. The patch seems to be calling parallel_vacuum_should_skip_index
> > > > > thrice even before starting parallel vacuum. It has a call to find the
> > > > > number of blocks which has to be performed for each index. I
> > > > > understand it might not be too costly to call this but it seems better
> > > > > to remember this info like we are doing in the current code.
> > > >
> > > > Yes, we can bring will_vacuum_parallel array back to the code. That
> > > > way, we can remove the call to parallel_vacuum_should_skip_index() in
> > > > parallel_vacuum_begin().
> > > >
> > > > > We can
> > > > > probably set parallel_workers_can_process in parallel_vacuum_begin and
> > > > > then again update in parallel_vacuum_process_all_indexes. Won't doing
> > > > > something like that be better?
> > > >
> > > > parallel_workers_can_process can vary depending on bulk-deletion, the
> > > > first time cleanup, or the second time (or more) cleanup. What can we
> > > > set parallel_workers_can_process based on in parallel_vacuum_begin()?
> > > >
> > >
> > > I was thinking to set the results of will_vacuum_parallel in
> > > parallel_vacuum_begin().
> > >
> >
> > This point doesn't seem to be addressed in the latest version (v6). Is
> > there a reason for not doing it? If we do this, then we don't need to
> > call parallel_vacuum_should_skip_index() from
> > parallel_vacuum_index_is_parallel_safe().
> >
>
> Few minor comments on v6-0001
> ==========================
> 1.
> The array
> + * element is allocated for every index, even those indexes where
> + * parallel index vacuuming is unsafe or not worthwhile (i.g.,
> + * parallel_vacuum_should_skip_index() returns true).
>
> /i.g/e.g
>
> 2.
> static void update_index_statistics(LVRelState *vacrel);
> -static void begin_parallel_vacuum(LVRelState *vacrel, int nrequested);
> -static void end_parallel_vacuum(LVRelState *vacrel);
> -static LVSharedIndStats *parallel_stats_for_idx(LVShared *lvshared,
> int getidx);
> -static bool parallel_processing_is_safe(Relation indrel, LVShared *lvshared);
> +
> +static int parallel_vacuum_compute_workers(LVRelState *vacrel, int nrequested,
> +    bool *will_parallel_vacuum);
>
> In declaration, parallel_vacuum_compute_workers() is declared after
> update_index_statistics but later defined in reverse order. I suggest
> to make the order of definitions same as their declaration. Similarly,
> the order of definition of parallel_vacuum_process_all_indexes(),
> parallel_vacuum_process_unsafe_indexes(),
> parallel_vacuum_process_safe_indexes(),
> parallel_vacuum_process_one_index() doesn't match the order of their
> declaration. Can we change that as well?

Agreed with the above two points.

I've attached updated patches that incorporated the above comments
too. Please review them.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  60. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-10T12:08:43Z

    On Thu, Dec 9, 2021 at 6:05 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Dec 9, 2021 at 7:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> Agreed with the above two points.
>
> I've attached updated patches that incorporated the above comments
> too. Please review them.
>

I have made the following minor changes to the 0001 patch: (a) An
assert was removed from dead_items_max_items() which I added back. (b)
Removed an unnecessary semicolon from one of the statements in
compute_parallel_vacuum_workers(). (c) Changed comments at a few
places. (d) moved all parallel_vacuum_* related functions together.
(e) ran pgindent and slightly modify the commit message.

Let me know what you think of the attached?

-- 
With Regards,
Amit Kapila.

  61. Re: parallel vacuum comments

    Andres Freund <andres@anarazel.de> — 2021-12-11T05:32:24Z

    Hi,

On 2021-10-30 14:21:01 -0700, Andres Freund wrote:
> Due to bug #17245: [1] I spent a considerably amount of time looking at vacuum
> related code. And I found a few things that I think could stand improvement:

While working on the fix for #17255 (more specifically some cleanup that Peter
suggested in the context), I noticed another thing: Initializing parallelism
as part of dead_items_alloc() is a bad idea. Even if there are comments noting
that oddity.

I don't really see why we should do it this way? There's no "no-parallelism"
path in begin_parallel_vacuum() besides compute_parallel_vacuum_workers(). So
it's not like we might just discover the inability to do parallelism during
parallel initialization?

It's also not particularly helpful to have a begin_parallel_vacuum() that
might not actually begin a parallel vacuum...


Minor nit:

begin_parallel_vacuum()'s comment says:
 * On success (when we can launch one or more workers), will set dead_items and
 * lps in vacrel for caller.

But it actually doesn't know whether we can start workers. It just checks
max_parallel_maintenance_workers, no?


Greetings,

Andres Freund

  62. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-11T14:59:31Z

    On Sat, Dec 11, 2021 at 2:32 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2021-10-30 14:21:01 -0700, Andres Freund wrote:
> > Due to bug #17245: [1] I spent a considerably amount of time looking at vacuum
> > related code. And I found a few things that I think could stand improvement:

Thank you for the comments.

>
> While working on the fix for #17255 (more specifically some cleanup that Peter
> suggested in the context), I noticed another thing: Initializing parallelism
> as part of dead_items_alloc() is a bad idea. Even if there are comments noting
> that oddity.
>
> I don't really see why we should do it this way? There's no "no-parallelism"
> path in begin_parallel_vacuum() besides compute_parallel_vacuum_workers(). So
> it's not like we might just discover the inability to do parallelism during
> parallel initialization?

Right. Also, in parallel vacuum case, it allocates the space not only
for dead items but also other data required to do parallelism like
shared bulkdeletion results etc. Originally, in PG13,
begin_parallel_vacuum() was called by lazy_scan_heap() but in PG14 it
became part of dead_items_alloc() (see b4af70cb2). I agree to change
this part so that lazy_scan_heap() calls begin_parallel_vacuum()
(whatever we rename it). I'll incorporate this change in the
refactoring patch barring any objections.

>
> It's also not particularly helpful to have a begin_parallel_vacuum() that
> might not actually begin a parallel vacuum...

During the development, I found that we have some begin_* functions
that don't start the actual parallel job but prepare state data for
starting parallel job and referred to  _bt_begin_parallel() so I named
begin_parallel_vacuum(). But I admit that considering what the
function actually does, something like
create_parallel_vacuum_context() would be clearer.

>
> Minor nit:
>
> begin_parallel_vacuum()'s comment says:
>  * On success (when we can launch one or more workers), will set dead_items and
>  * lps in vacrel for caller.
>
> But it actually doesn't know whether we can start workers. It just checks
> max_parallel_maintenance_workers, no?

Yes, we cannot know whether we can actually start workers when
starting parallel index vacuuming. It returns non-NULL if we request
one or more workers.

Regards

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  63. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-13T03:02:55Z

    On Sat, Dec 11, 2021 at 8:30 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sat, Dec 11, 2021 at 2:32 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > On 2021-10-30 14:21:01 -0700, Andres Freund wrote:
> > > Due to bug #17245: [1] I spent a considerably amount of time looking at vacuum
> > > related code. And I found a few things that I think could stand improvement:
>
> Thank you for the comments.
>
> >
> > While working on the fix for #17255 (more specifically some cleanup that Peter
> > suggested in the context), I noticed another thing: Initializing parallelism
> > as part of dead_items_alloc() is a bad idea. Even if there are comments noting
> > that oddity.
> >
> > I don't really see why we should do it this way? There's no "no-parallelism"
> > path in begin_parallel_vacuum() besides compute_parallel_vacuum_workers(). So
> > it's not like we might just discover the inability to do parallelism during
> > parallel initialization?
>
> Right. Also, in parallel vacuum case, it allocates the space not only
> for dead items but also other data required to do parallelism like
> shared bulkdeletion results etc. Originally, in PG13,
> begin_parallel_vacuum() was called by lazy_scan_heap() but in PG14 it
> became part of dead_items_alloc() (see b4af70cb2). I agree to change
> this part so that lazy_scan_heap() calls begin_parallel_vacuum()
> (whatever we rename it). I'll incorporate this change in the
> refactoring patch barring any objections.
>
> >
> > It's also not particularly helpful to have a begin_parallel_vacuum() that
> > might not actually begin a parallel vacuum...
>
> During the development, I found that we have some begin_* functions
> that don't start the actual parallel job but prepare state data for
> starting parallel job and referred to  _bt_begin_parallel() so I named
> begin_parallel_vacuum(). But I admit that considering what the
> function actually does, something like
> create_parallel_vacuum_context() would be clearer.
>

How about if we name it as parallel_vacuum_init() which will be
similar InitializeParallelDSM, ExecInitParallelPlan(). Now, I see
there is some reasoning to keep it in dead_items_alloc as both
primarily allocate memory for vacuum but maybe we should name the
function vacuum_space_alloc instead of dead_items_alloc and similarly
rename dead_items_cleanup to vacuum_space_free. The other idea could
be to bring begin_parallel_vacuum() back in lazy_scan_heap() but I
personally prefer the idea to keep it where it is but improve function
names. Will it be better to do this as a separate patch as 0002
because this might require some change in the vacuum code path?

> >
> > Minor nit:
> >
> > begin_parallel_vacuum()'s comment says:
> >  * On success (when we can launch one or more workers), will set dead_items and
> >  * lps in vacrel for caller.
> >
> > But it actually doesn't know whether we can start workers. It just checks
> > max_parallel_maintenance_workers, no?
>
> Yes, we cannot know whether we can actually start workers when
> starting parallel index vacuuming. It returns non-NULL if we request
> one or more workers.
>

So can we adjust the comments? I think the part of the sentence "when
we can launch one or more workers" seems to be the cause of confusion,
can we remove it?

-- 
With Regards,
Amit Kapila.

  64. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-13T04:54:43Z

    On Mon, Dec 13, 2021 at 12:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Dec 11, 2021 at 8:30 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Sat, Dec 11, 2021 at 2:32 PM Andres Freund <andres@anarazel.de> wrote:
> > >
> > > Hi,
> > >
> > > On 2021-10-30 14:21:01 -0700, Andres Freund wrote:
> > > > Due to bug #17245: [1] I spent a considerably amount of time looking at vacuum
> > > > related code. And I found a few things that I think could stand improvement:
> >
> > Thank you for the comments.
> >
> > >
> > > While working on the fix for #17255 (more specifically some cleanup that Peter
> > > suggested in the context), I noticed another thing: Initializing parallelism
> > > as part of dead_items_alloc() is a bad idea. Even if there are comments noting
> > > that oddity.
> > >
> > > I don't really see why we should do it this way? There's no "no-parallelism"
> > > path in begin_parallel_vacuum() besides compute_parallel_vacuum_workers(). So
> > > it's not like we might just discover the inability to do parallelism during
> > > parallel initialization?
> >
> > Right. Also, in parallel vacuum case, it allocates the space not only
> > for dead items but also other data required to do parallelism like
> > shared bulkdeletion results etc. Originally, in PG13,
> > begin_parallel_vacuum() was called by lazy_scan_heap() but in PG14 it
> > became part of dead_items_alloc() (see b4af70cb2). I agree to change
> > this part so that lazy_scan_heap() calls begin_parallel_vacuum()
> > (whatever we rename it). I'll incorporate this change in the
> > refactoring patch barring any objections.
> >
> > >
> > > It's also not particularly helpful to have a begin_parallel_vacuum() that
> > > might not actually begin a parallel vacuum...
> >
> > During the development, I found that we have some begin_* functions
> > that don't start the actual parallel job but prepare state data for
> > starting parallel job and referred to  _bt_begin_parallel() so I named
> > begin_parallel_vacuum(). But I admit that considering what the
> > function actually does, something like
> > create_parallel_vacuum_context() would be clearer.
> >
>
> How about if we name it as parallel_vacuum_init() which will be
> similar InitializeParallelDSM, ExecInitParallelPlan().

parallel_vacuum_init() sounds better.

> Now, I see
> there is some reasoning to keep it in dead_items_alloc as both
> primarily allocate memory for vacuum but maybe we should name the
> function vacuum_space_alloc instead of dead_items_alloc and similarly
> rename dead_items_cleanup to vacuum_space_free. The other idea could
> be to bring begin_parallel_vacuum() back in lazy_scan_heap() but I
> personally prefer the idea to keep it where it is but improve function
> names. Will it be better to do this as a separate patch as 0002
> because this might require some change in the vacuum code path?

Yeah, if we do just renaming functions, I think we can do that in 0001
patch. On the other hand, if we need to change the logic, it's better
to do that in a separate patch.

>
> > >
> > > Minor nit:
> > >
> > > begin_parallel_vacuum()'s comment says:
> > >  * On success (when we can launch one or more workers), will set dead_items and
> > >  * lps in vacrel for caller.
> > >
> > > But it actually doesn't know whether we can start workers. It just checks
> > > max_parallel_maintenance_workers, no?
> >
> > Yes, we cannot know whether we can actually start workers when
> > starting parallel index vacuuming. It returns non-NULL if we request
> > one or more workers.
> >
>
> So can we adjust the comments? I think the part of the sentence "when
> we can launch one or more workers" seems to be the cause of confusion,
> can we remove it?

Yes, we can remove it. Or replace "can launch" with "request".

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  65. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-13T05:02:55Z

    On Fri, Dec 10, 2021 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Dec 9, 2021 at 6:05 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Dec 9, 2021 at 7:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Agreed with the above two points.
> >
> > I've attached updated patches that incorporated the above comments
> > too. Please review them.
> >
>
> I have made the following minor changes to the 0001 patch: (a) An
> assert was removed from dead_items_max_items() which I added back. (b)
> Removed an unnecessary semicolon from one of the statements in
> compute_parallel_vacuum_workers(). (c) Changed comments at a few
> places. (d) moved all parallel_vacuum_* related functions together.
> (e) ran pgindent and slightly modify the commit message.
>
> Let me know what you think of the attached?

Thank you for updating the patch!

The patch also moves some functions, e.g., update_index_statistics()
is moved without code changes. I agree to move functions for
consistency but that makes the review hard and the patch complicated.
I think it's better to do improving the parallel vacuum code and
moving functions in separate patches.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  66. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-13T05:09:35Z

    On Mon, Dec 13, 2021 at 10:33 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Dec 10, 2021 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Dec 9, 2021 at 6:05 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Thu, Dec 9, 2021 at 7:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > Agreed with the above two points.
> > >
> > > I've attached updated patches that incorporated the above comments
> > > too. Please review them.
> > >
> >
> > I have made the following minor changes to the 0001 patch: (a) An
> > assert was removed from dead_items_max_items() which I added back. (b)
> > Removed an unnecessary semicolon from one of the statements in
> > compute_parallel_vacuum_workers(). (c) Changed comments at a few
> > places. (d) moved all parallel_vacuum_* related functions together.
> > (e) ran pgindent and slightly modify the commit message.
> >
> > Let me know what you think of the attached?
>
> Thank you for updating the patch!
>
> The patch also moves some functions, e.g., update_index_statistics()
> is moved without code changes. I agree to move functions for
> consistency but that makes the review hard and the patch complicated.
> I think it's better to do improving the parallel vacuum code and
> moving functions in separate patches.
>

Okay, I thought it might be better to keep all parallel_vacuum_*
related functions together but we can keep that in a separate patch
Feel free to submit without those changes. In fact, if we go for your
current 0002 then that might not be even required as we move all those
functions to a new file.

-- 
With Regards,
Amit Kapila.

  67. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-13T06:12:14Z

    On Mon, Dec 13, 2021 at 2:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Dec 13, 2021 at 10:33 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Dec 10, 2021 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Dec 9, 2021 at 6:05 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Thu, Dec 9, 2021 at 7:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > Agreed with the above two points.
> > > >
> > > > I've attached updated patches that incorporated the above comments
> > > > too. Please review them.
> > > >
> > >
> > > I have made the following minor changes to the 0001 patch: (a) An
> > > assert was removed from dead_items_max_items() which I added back. (b)
> > > Removed an unnecessary semicolon from one of the statements in
> > > compute_parallel_vacuum_workers(). (c) Changed comments at a few
> > > places. (d) moved all parallel_vacuum_* related functions together.
> > > (e) ran pgindent and slightly modify the commit message.
> > >
> > > Let me know what you think of the attached?
> >
> > Thank you for updating the patch!
> >
> > The patch also moves some functions, e.g., update_index_statistics()
> > is moved without code changes. I agree to move functions for
> > consistency but that makes the review hard and the patch complicated.
> > I think it's better to do improving the parallel vacuum code and
> > moving functions in separate patches.
> >
>
> Okay, I thought it might be better to keep all parallel_vacuum_*
> related functions together but we can keep that in a separate patch
> Feel free to submit without those changes.

I've attached the patch. I've just moved some functions back but not
done other changes.

> In fact, if we go for your
> current 0002 then that might not be even required as we move all those
> functions to a new file.

Right. So it seems not necessary.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  68. RE: parallel vacuum comments

    tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2021-12-14T02:10:33Z

    On Monday, December 13, 2021 2:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> 
> On Mon, Dec 13, 2021 at 2:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Dec 13, 2021 at 10:33 AM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> > >
> > > On Fri, Dec 10, 2021 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > > >
> > > > On Thu, Dec 9, 2021 at 6:05 PM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> > > > >
> > > > > On Thu, Dec 9, 2021 at 7:44 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > > > >
> > > > > Agreed with the above two points.
> > > > >
> > > > > I've attached updated patches that incorporated the above comments
> > > > > too. Please review them.
> > > > >
> > > >
> > > > I have made the following minor changes to the 0001 patch: (a) An
> > > > assert was removed from dead_items_max_items() which I added back. (b)
> > > > Removed an unnecessary semicolon from one of the statements in
> > > > compute_parallel_vacuum_workers(). (c) Changed comments at a few
> > > > places. (d) moved all parallel_vacuum_* related functions together.
> > > > (e) ran pgindent and slightly modify the commit message.
> > > >
> > > > Let me know what you think of the attached?
> > >
> > > Thank you for updating the patch!
> > >
> > > The patch also moves some functions, e.g., update_index_statistics()
> > > is moved without code changes. I agree to move functions for
> > > consistency but that makes the review hard and the patch complicated.
> > > I think it's better to do improving the parallel vacuum code and
> > > moving functions in separate patches.
> > >
> >
> > Okay, I thought it might be better to keep all parallel_vacuum_*
> > related functions together but we can keep that in a separate patch
> > Feel free to submit without those changes.
> 
> I've attached the patch. I've just moved some functions back but not
> done other changes.
> 

Thanks for your patch.

I tested your patch and tried some cases, like large indexes, different types of indexes, it worked well.

Besides, I noticed a typo as follows:

+	/* Estimate size for index vacuum stats -- PARALLEL_VACUUM_KEY_STATS */

"PARALLEL_VACUUM_KEY_STATS" should be "PARALLEL_VACUUM_KEY_INDEX_STATS".

Regards,
Tang

  69. RE: parallel vacuum comments

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-12-14T02:23:14Z

    On Tuesday, December 14, 2021 10:11 AM Tang, Haiying wrote:
> On Monday, December 13, 2021 2:12 PM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> > I've attached the patch. I've just moved some functions back but not
> > done other changes.
> >
> 
> Thanks for your patch.
> 
> I tested your patch and tried some cases, like large indexes, different types of
> indexes, it worked well.

+1, the patch looks good to me.

Best regards,
Hou zj

  70. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-14T03:02:53Z

    On Tue, Dec 14, 2021 at 7:40 AM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:
>
> On Monday, December 13, 2021 2:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Dec 13, 2021 at 2:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Dec 13, 2021 at 10:33 AM Masahiko Sawada
> > <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Fri, Dec 10, 2021 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com>
> > wrote:
> > > > >
> > > > > On Thu, Dec 9, 2021 at 6:05 PM Masahiko Sawada
> > <sawada.mshk@gmail.com> wrote:
> > > > > >
> > > > > > On Thu, Dec 9, 2021 at 7:44 PM Amit Kapila <amit.kapila16@gmail.com>
> > wrote:
> > > > > >
> > > > > > Agreed with the above two points.
> > > > > >
> > > > > > I've attached updated patches that incorporated the above comments
> > > > > > too. Please review them.
> > > > > >
> > > > >
> > > > > I have made the following minor changes to the 0001 patch: (a) An
> > > > > assert was removed from dead_items_max_items() which I added back. (b)
> > > > > Removed an unnecessary semicolon from one of the statements in
> > > > > compute_parallel_vacuum_workers(). (c) Changed comments at a few
> > > > > places. (d) moved all parallel_vacuum_* related functions together.
> > > > > (e) ran pgindent and slightly modify the commit message.
> > > > >
> > > > > Let me know what you think of the attached?
> > > >
> > > > Thank you for updating the patch!
> > > >
> > > > The patch also moves some functions, e.g., update_index_statistics()
> > > > is moved without code changes. I agree to move functions for
> > > > consistency but that makes the review hard and the patch complicated.
> > > > I think it's better to do improving the parallel vacuum code and
> > > > moving functions in separate patches.
> > > >
> > >
> > > Okay, I thought it might be better to keep all parallel_vacuum_*
> > > related functions together but we can keep that in a separate patch
> > > Feel free to submit without those changes.
> >
> > I've attached the patch. I've just moved some functions back but not
> > done other changes.
> >
>
> Thanks for your patch.
>
> I tested your patch and tried some cases, like large indexes, different types of indexes, it worked well.
>
> Besides, I noticed a typo as follows:
>
> +       /* Estimate size for index vacuum stats -- PARALLEL_VACUUM_KEY_STATS */
>
> "PARALLEL_VACUUM_KEY_STATS" should be "PARALLEL_VACUUM_KEY_INDEX_STATS".
>

Thanks, I can take care of this before committing. The v9-0001* looks
good to me as well, so, I am planning to commit that tomorrow unless I
see more comments or any objection to that. There is still pending
work related to moving parallel vacuum code to a separate file and a
few other pending comments that are still under discussion. We can
take care of those in subsequent patches. Do, let me know if you or
others think differently?

-- 
With Regards,
Amit Kapila.

  71. Re: parallel vacuum comments

    Peter Geoghegan <pg@bowt.ie> — 2021-12-15T02:53:12Z

    On Mon, Dec 13, 2021 at 7:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Thanks, I can take care of this before committing. The v9-0001* looks
> good to me as well, so, I am planning to commit that tomorrow unless I
> see more comments or any objection to that.

I would like to thank both Masahiko and yourself for working on this.
It's important.

> There is still pending
> work related to moving parallel vacuum code to a separate file and a
> few other pending comments that are still under discussion. We can
> take care of those in subsequent patches. Do, let me know if you or
> others think differently?

I'm +1 on moving it into a new file. I think that that division makes
perfect sense. It will make the design of parallel VACUUM easier to
understand. I believe that index vacuuming (whether or not it involves
parallel workers) ought to be a more or less distinct operation to
heap vacuuming. With a distinct autovacuum schedule (well, the
schedule would be related, but still distinct).

-- 
Peter Geoghegan

  72. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-15T03:01:19Z

    On Wed, Dec 15, 2021 at 8:23 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Mon, Dec 13, 2021 at 7:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Thanks, I can take care of this before committing. The v9-0001* looks
> > good to me as well, so, I am planning to commit that tomorrow unless I
> > see more comments or any objection to that.
>
> I would like to thank both Masahiko and yourself for working on this.
> It's important.
>
> > There is still pending
> > work related to moving parallel vacuum code to a separate file and a
> > few other pending comments that are still under discussion. We can
> > take care of those in subsequent patches. Do, let me know if you or
> > others think differently?
>
> I'm +1 on moving it into a new file. I think that that division makes
> perfect sense. It will make the design of parallel VACUUM easier to
> understand.
>

Agreed and thanks for your support.

-- 
With Regards,
Amit Kapila.

  73. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-15T08:02:51Z

    On Tue, Dec 14, 2021 at 12:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Dec 14, 2021 at 7:40 AM tanghy.fnst@fujitsu.com
> <tanghy.fnst@fujitsu.com> wrote:
> >
> > On Monday, December 13, 2021 2:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Mon, Dec 13, 2021 at 2:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Mon, Dec 13, 2021 at 10:33 AM Masahiko Sawada
> > > <sawada.mshk@gmail.com> wrote:
> > > > >
> > > > > On Fri, Dec 10, 2021 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com>
> > > wrote:
> > > > > >
> > > > > > On Thu, Dec 9, 2021 at 6:05 PM Masahiko Sawada
> > > <sawada.mshk@gmail.com> wrote:
> > > > > > >
> > > > > > > On Thu, Dec 9, 2021 at 7:44 PM Amit Kapila <amit.kapila16@gmail.com>
> > > wrote:
> > > > > > >
> > > > > > > Agreed with the above two points.
> > > > > > >
> > > > > > > I've attached updated patches that incorporated the above comments
> > > > > > > too. Please review them.
> > > > > > >
> > > > > >
> > > > > > I have made the following minor changes to the 0001 patch: (a) An
> > > > > > assert was removed from dead_items_max_items() which I added back. (b)
> > > > > > Removed an unnecessary semicolon from one of the statements in
> > > > > > compute_parallel_vacuum_workers(). (c) Changed comments at a few
> > > > > > places. (d) moved all parallel_vacuum_* related functions together.
> > > > > > (e) ran pgindent and slightly modify the commit message.
> > > > > >
> > > > > > Let me know what you think of the attached?
> > > > >
> > > > > Thank you for updating the patch!
> > > > >
> > > > > The patch also moves some functions, e.g., update_index_statistics()
> > > > > is moved without code changes. I agree to move functions for
> > > > > consistency but that makes the review hard and the patch complicated.
> > > > > I think it's better to do improving the parallel vacuum code and
> > > > > moving functions in separate patches.
> > > > >
> > > >
> > > > Okay, I thought it might be better to keep all parallel_vacuum_*
> > > > related functions together but we can keep that in a separate patch
> > > > Feel free to submit without those changes.
> > >
> > > I've attached the patch. I've just moved some functions back but not
> > > done other changes.
> > >
> >
> > Thanks for your patch.
> >
> > I tested your patch and tried some cases, like large indexes, different types of indexes, it worked well.
> >
> > Besides, I noticed a typo as follows:
> >
> > +       /* Estimate size for index vacuum stats -- PARALLEL_VACUUM_KEY_STATS */
> >
> > "PARALLEL_VACUUM_KEY_STATS" should be "PARALLEL_VACUUM_KEY_INDEX_STATS".
> >
>
> Thanks, I can take care of this before committing. The v9-0001* looks
> good to me as well, so, I am planning to commit that tomorrow unless I
> see more comments or any objection to that.

Thanks!

> There is still pending
> work related to moving parallel vacuum code to a separate file and a
> few other pending comments that are still under discussion. We can
> take care of those in subsequent patches. Do, let me know if you or
> others think differently?

I'm on the same page.

I've attached an updated patch. The patch incorporated several changes
from the last version:

* Rename parallel_vacuum_begin() to parallel_vacuum_init()
* Unify the terminology; use "index bulk-deletion" and "index cleanup"
instead of "index vacuum" and "index cleanup".
* Fix the comment of parallel_vacuum_init() pointed out by Andres
* Fix a typo that is left in commit 22bd3cbe0c (pointed out by Hou)

Please review it.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  74. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-16T04:55:52Z

    On Wed, Dec 15, 2021 at 1:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've attached an updated patch. The patch incorporated several changes
> from the last version:
>
> * Rename parallel_vacuum_begin() to parallel_vacuum_init()
> * Unify the terminology; use "index bulk-deletion" and "index cleanup"
> instead of "index vacuum" and "index cleanup".
>

I am not sure it is a good idea to do this as part of the main patch
as the intention of that is to just refactor parallel vacuum code. I
suggest doing this as a separate patch. Also, can we move the common
code to be shared between vacuumparallel.c and vacuumlazy.c as a
separate patch?

Few other comments and questions:
============================
1. /* Outsource everything to parallel variant */
- parallel_vacuum_process_all_indexes(vacrel, true);
+ LVSavedErrInfo saved_err_info;
+
+ /*
+ * Outsource everything to parallel variant. Since parallel vacuum will
+ * set the error context on an error we temporarily disable setting our
+ * error context.
+ */
+ update_vacuum_error_info(vacrel, &saved_err_info,
+ VACUUM_ERRCB_PHASE_UNKNOWN,
+ InvalidBlockNumber, InvalidOffsetNumber);
+
+ parallel_vacuum_bulkdel_all_indexes(vacrel->pvs, vacrel->old_live_tuples);
+
+ /* Revert to the previous phase information for error traceback */
+ restore_vacuum_error_info(vacrel, &saved_err_info);

Is this change because you want a separate error callback for parallel
vacuum? If so, I suggest we can discuss this as a separate patch from
the refactoring patch.

2. Is introducing bulkdel_one_index/cleanup_one_index related to new
error context, or "Unify the terminology" task? Is there any other
reason for the same?

3. Why did you introduce
parallel_vacuum_bulkdel_all_indexes()/parallel_vacuum_cleanup_all_indexes()?
Is it because of your task "Unify the terminology"?

4.
@@ -3086,7 +2592,6 @@ lazy_cleanup_one_index(Relation indrel,
IndexBulkDeleteResult *istat,
  ivinfo.report_progress = false;
  ivinfo.estimated_count = estimated_count;
  ivinfo.message_level = elevel;
-
  ivinfo.num_heap_tuples = reltuples;

This seems like an unrelated change.

-- 
With Regards,
Amit Kapila.

  75. RE: parallel vacuum comments

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-12-16T07:27:02Z

    On Wed, Dec 15, 2021 4:03 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Tue, Dec 14, 2021 at 12:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > There is still pending
> > work related to moving parallel vacuum code to a separate file and a
> > few other pending comments that are still under discussion. We can
> > take care of those in subsequent patches. Do, let me know if you or
> > others think differently?
> 
> I'm on the same page.
> 
> I've attached an updated patch. The patch incorporated several changes from
> the last version:
> 
> * Rename parallel_vacuum_begin() to parallel_vacuum_init()
> * Unify the terminology; use "index bulk-deletion" and "index cleanup"
> instead of "index vacuum" and "index cleanup".
> * Fix the comment of parallel_vacuum_init() pointed out by Andres
> * Fix a typo that is left in commit 22bd3cbe0c (pointed out by Hou)
> 
> Please review it.

Thanks for updating the patch.
Here are a few comments:

1)
+		case PARALLEL_INDVAC_STATUS_NEED_CLEANUP:
+			errcontext("while cleanup index \"%s\" of relation \"%s.%s\"",

I noticed current code uses error msg "While cleaning up index xxx" which seems a little
different from the patch's maybe we can use the previous one ?

2)
static inline Size max_items_to_alloc_size(int max_items);

This old function declaration can be deleted.

3)
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list

I think we need to remove LVShared, LVSharedIndStats, LVDeadItems and
LVParallelState from typedefs.list and add PVShared and PVIndStats to the file.

Best regards,
Hou zj

  76. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-16T12:42:54Z

    On Thu, Dec 16, 2021 at 1:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Dec 15, 2021 at 1:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached an updated patch. The patch incorporated several changes
> > from the last version:
> >
> > * Rename parallel_vacuum_begin() to parallel_vacuum_init()
> > * Unify the terminology; use "index bulk-deletion" and "index cleanup"
> > instead of "index vacuum" and "index cleanup".
> >
>
> I am not sure it is a good idea to do this as part of the main patch
> as the intention of that is to just refactor parallel vacuum code. I
> suggest doing this as a separate patch.

Okay.

>  Also, can we move the common
> code to be shared between vacuumparallel.c and vacuumlazy.c as a
> separate patch?

You mean vac_tid_reaped() and vac_cmp_itemptr() etc.? If so, do both
vacuumparallel.c and vacuumlazy.c have the same functions?

>
> Few other comments and questions:
> ============================
> 1. /* Outsource everything to parallel variant */
> - parallel_vacuum_process_all_indexes(vacrel, true);
> + LVSavedErrInfo saved_err_info;
> +
> + /*
> + * Outsource everything to parallel variant. Since parallel vacuum will
> + * set the error context on an error we temporarily disable setting our
> + * error context.
> + */
> + update_vacuum_error_info(vacrel, &saved_err_info,
> + VACUUM_ERRCB_PHASE_UNKNOWN,
> + InvalidBlockNumber, InvalidOffsetNumber);
> +
> + parallel_vacuum_bulkdel_all_indexes(vacrel->pvs, vacrel->old_live_tuples);
> +
> + /* Revert to the previous phase information for error traceback */
> + restore_vacuum_error_info(vacrel, &saved_err_info);
>
> Is this change because you want a separate error callback for parallel
> vacuum? If so, I suggest we can discuss this as a separate patch from
> the refactoring patch.

Because it seems natural to me that the leader and worker use the same
error callback.

Okay, I'll remove that change in the next version patch.

> 2. Is introducing bulkdel_one_index/cleanup_one_index related to new
> error context, or "Unify the terminology" task? Is there any other
> reason for the same?

Because otherwise both vacuumlazy.c and vacuumparallel.c will have the
same functions.

>
> 3. Why did you introduce
> parallel_vacuum_bulkdel_all_indexes()/parallel_vacuum_cleanup_all_indexes()?
> Is it because of your task "Unify the terminology"?

This is because parallel bulk-deletion and cleanup require different
numbers of inputs (num_table_tuples etc.) and the caller
(vacuumlazy.c) cannot set them directly to ParallelVacuumState.

>
> 4.
> @@ -3086,7 +2592,6 @@ lazy_cleanup_one_index(Relation indrel,
> IndexBulkDeleteResult *istat,
>   ivinfo.report_progress = false;
>   ivinfo.estimated_count = estimated_count;
>   ivinfo.message_level = elevel;
> -
>   ivinfo.num_heap_tuples = reltuples;
>
> This seems like an unrelated change.

Yes, but I think it's an unnecessary break so we can change it
together. Should it be done in a separate patch?

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  77. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-16T13:38:12Z

    On Thu, Dec 16, 2021 at 6:13 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Dec 16, 2021 at 1:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Dec 15, 2021 at 1:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > I've attached an updated patch. The patch incorporated several changes
> > > from the last version:
> > >
> > > * Rename parallel_vacuum_begin() to parallel_vacuum_init()
> > > * Unify the terminology; use "index bulk-deletion" and "index cleanup"
> > > instead of "index vacuum" and "index cleanup".
> > >
> >
> > I am not sure it is a good idea to do this as part of the main patch
> > as the intention of that is to just refactor parallel vacuum code. I
> > suggest doing this as a separate patch.
>
> Okay.
>
> >  Also, can we move the common
> > code to be shared between vacuumparallel.c and vacuumlazy.c as a
> > separate patch?
>
> You mean vac_tid_reaped() and vac_cmp_itemptr() etc.? If so, do both
> vacuumparallel.c and vacuumlazy.c have the same functions?
>

Why that would be required? I think both can call the common exposed
function like the one you have in your patch bulkdel_one_index or if
we directly move lazy_vacuum_one_index as part of common code. Similar
for cleanup function.

> >
> > Few other comments and questions:
> > ============================
> > 1. /* Outsource everything to parallel variant */
> > - parallel_vacuum_process_all_indexes(vacrel, true);
> > + LVSavedErrInfo saved_err_info;
> > +
> > + /*
> > + * Outsource everything to parallel variant. Since parallel vacuum will
> > + * set the error context on an error we temporarily disable setting our
> > + * error context.
> > + */
> > + update_vacuum_error_info(vacrel, &saved_err_info,
> > + VACUUM_ERRCB_PHASE_UNKNOWN,
> > + InvalidBlockNumber, InvalidOffsetNumber);
> > +
> > + parallel_vacuum_bulkdel_all_indexes(vacrel->pvs, vacrel->old_live_tuples);
> > +
> > + /* Revert to the previous phase information for error traceback */
> > + restore_vacuum_error_info(vacrel, &saved_err_info);
> >
> > Is this change because you want a separate error callback for parallel
> > vacuum? If so, I suggest we can discuss this as a separate patch from
> > the refactoring patch.
>
> Because it seems natural to me that the leader and worker use the same
> error callback.
>
> Okay, I'll remove that change in the next version patch.
>
> > 2. Is introducing bulkdel_one_index/cleanup_one_index related to new
> > error context, or "Unify the terminology" task? Is there any other
> > reason for the same?
>
> Because otherwise both vacuumlazy.c and vacuumparallel.c will have the
> same functions.
>
> >
> > 3. Why did you introduce
> > parallel_vacuum_bulkdel_all_indexes()/parallel_vacuum_cleanup_all_indexes()?
> > Is it because of your task "Unify the terminology"?
>
> This is because parallel bulk-deletion and cleanup require different
> numbers of inputs (num_table_tuples etc.) and the caller
> (vacuumlazy.c) cannot set them directly to ParallelVacuumState.
>

oh, yeah, the other possibility could be to have a common structure
that can be used for both cases. I am not sure if that is better than
what you have.

> >
> > 4.
> > @@ -3086,7 +2592,6 @@ lazy_cleanup_one_index(Relation indrel,
> > IndexBulkDeleteResult *istat,
> >   ivinfo.report_progress = false;
> >   ivinfo.estimated_count = estimated_count;
> >   ivinfo.message_level = elevel;
> > -
> >   ivinfo.num_heap_tuples = reltuples;
> >
> > This seems like an unrelated change.
>
> Yes, but I think it's an unnecessary break so we can change it
> together. Should it be done in a separate patch?
>

Isn't this just spurious line removal which shouldn't be part of any patch?

-- 
With Regards,
Amit Kapila.

  78. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-17T05:20:58Z

    On Thu, Dec 16, 2021 at 10:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Dec 16, 2021 at 6:13 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Dec 16, 2021 at 1:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Dec 15, 2021 at 1:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > I've attached an updated patch. The patch incorporated several changes
> > > > from the last version:
> > > >
> > > > * Rename parallel_vacuum_begin() to parallel_vacuum_init()
> > > > * Unify the terminology; use "index bulk-deletion" and "index cleanup"
> > > > instead of "index vacuum" and "index cleanup".
> > > >
> > >
> > > I am not sure it is a good idea to do this as part of the main patch
> > > as the intention of that is to just refactor parallel vacuum code. I
> > > suggest doing this as a separate patch.
> >
> > Okay.
> >
> > >  Also, can we move the common
> > > code to be shared between vacuumparallel.c and vacuumlazy.c as a
> > > separate patch?
> >
> > You mean vac_tid_reaped() and vac_cmp_itemptr() etc.? If so, do both
> > vacuumparallel.c and vacuumlazy.c have the same functions?
> >
>
> Why that would be required? I think both can call the common exposed
> function like the one you have in your patch bulkdel_one_index or if
> we directly move lazy_vacuum_one_index as part of common code. Similar
> for cleanup function.

Understood.

>
> > >
> > > Few other comments and questions:
> > > ============================
> > > 1. /* Outsource everything to parallel variant */
> > > - parallel_vacuum_process_all_indexes(vacrel, true);
> > > + LVSavedErrInfo saved_err_info;
> > > +
> > > + /*
> > > + * Outsource everything to parallel variant. Since parallel vacuum will
> > > + * set the error context on an error we temporarily disable setting our
> > > + * error context.
> > > + */
> > > + update_vacuum_error_info(vacrel, &saved_err_info,
> > > + VACUUM_ERRCB_PHASE_UNKNOWN,
> > > + InvalidBlockNumber, InvalidOffsetNumber);
> > > +
> > > + parallel_vacuum_bulkdel_all_indexes(vacrel->pvs, vacrel->old_live_tuples);
> > > +
> > > + /* Revert to the previous phase information for error traceback */
> > > + restore_vacuum_error_info(vacrel, &saved_err_info);
> > >
> > > Is this change because you want a separate error callback for parallel
> > > vacuum? If so, I suggest we can discuss this as a separate patch from
> > > the refactoring patch.
> >
> > Because it seems natural to me that the leader and worker use the same
> > error callback.
> >
> > Okay, I'll remove that change in the next version patch.
> >
> > > 2. Is introducing bulkdel_one_index/cleanup_one_index related to new
> > > error context, or "Unify the terminology" task? Is there any other
> > > reason for the same?
> >
> > Because otherwise both vacuumlazy.c and vacuumparallel.c will have the
> > same functions.
> >
> > >
> > > 3. Why did you introduce
> > > parallel_vacuum_bulkdel_all_indexes()/parallel_vacuum_cleanup_all_indexes()?
> > > Is it because of your task "Unify the terminology"?
> >
> > This is because parallel bulk-deletion and cleanup require different
> > numbers of inputs (num_table_tuples etc.) and the caller
> > (vacuumlazy.c) cannot set them directly to ParallelVacuumState.
> >
>
> oh, yeah, the other possibility could be to have a common structure
> that can be used for both cases. I am not sure if that is better than
> what you have.

Yes, I left them as they are in an updated patch for now. But we can
change them if others think it’s not a good idea.

>
> > >
> > > 4.
> > > @@ -3086,7 +2592,6 @@ lazy_cleanup_one_index(Relation indrel,
> > > IndexBulkDeleteResult *istat,
> > >   ivinfo.report_progress = false;
> > >   ivinfo.estimated_count = estimated_count;
> > >   ivinfo.message_level = elevel;
> > > -
> > >   ivinfo.num_heap_tuples = reltuples;
> > >
> > > This seems like an unrelated change.
> >
> > Yes, but I think it's an unnecessary break so we can change it
> > together. Should it be done in a separate patch?
> >
>
> Isn't this just spurious line removal which shouldn't be part of any patch?

Okay.

I've attached updated patches. The first patch just moves common
function for index bulk-deletion and cleanup to vacuum.c. And the
second patch moves parallel vacuum code to vacuumparallel.c. The
comments I got so far are incorporated into these patches. Please
review them.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  79. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-17T05:30:31Z

    On Thu, Dec 16, 2021 at 4:27 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Wed, Dec 15, 2021 4:03 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > On Tue, Dec 14, 2021 at 12:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > There is still pending
> > > work related to moving parallel vacuum code to a separate file and a
> > > few other pending comments that are still under discussion. We can
> > > take care of those in subsequent patches. Do, let me know if you or
> > > others think differently?
> >
> > I'm on the same page.
> >
> > I've attached an updated patch. The patch incorporated several changes from
> > the last version:
> >
> > * Rename parallel_vacuum_begin() to parallel_vacuum_init()
> > * Unify the terminology; use "index bulk-deletion" and "index cleanup"
> > instead of "index vacuum" and "index cleanup".
> > * Fix the comment of parallel_vacuum_init() pointed out by Andres
> > * Fix a typo that is left in commit 22bd3cbe0c (pointed out by Hou)
> >
> > Please review it.
>
> Thanks for updating the patch.
> Here are a few comments:

Thank you for the comments!

>
> 1)
> +               case PARALLEL_INDVAC_STATUS_NEED_CLEANUP:
> +                       errcontext("while cleanup index \"%s\" of relation \"%s.%s\"",
>
> I noticed current code uses error msg "While cleaning up index xxx" which seems a little
> different from the patch's maybe we can use the previous one ?

Right. Fixed.

>
> 2)
> static inline Size max_items_to_alloc_size(int max_items);
>
> This old function declaration can be deleted.

Removed.

>
> 3)
> --- a/src/tools/pgindent/typedefs.list
> +++ b/src/tools/pgindent/typedefs.list
>
> I think we need to remove LVShared, LVSharedIndStats, LVDeadItems and
> LVParallelState from typedefs.list and add PVShared and PVIndStats to the file.

Fixed.

These comments are incorporated into the patch I just submitted[1].

Regards,

[1] https://www.postgresql.org/message-id/CAD21AoB66GqEjHttbRd0_fy9hnBPJp8kBCWnMq87mG6V_BODSA%40mail.gmail.com

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  80. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-18T06:38:34Z

    On Fri, Dec 17, 2021 at 10:51 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've attached updated patches. The first patch just moves common
> function for index bulk-deletion and cleanup to vacuum.c. And the
> second patch moves parallel vacuum code to vacuumparallel.c. The
> comments I got so far are incorporated into these patches. Please
> review them.
>

Thanks, it is helpful for the purpose of review.

Few comments:
=============
1.
+ * dead_items stores TIDs whose index tuples are deleted by index vacuuming.
+ * Each TID points to an LP_DEAD line pointer from a heap page that has been
+ * processed by lazy_scan_prune.  Also needed by lazy_vacuum_heap_rel, which
+ * marks the same LP_DEAD line pointers as LP_UNUSED during second heap pass.
  */
- LVDeadItems *dead_items; /* TIDs whose index tuples we'll delete */
+ VacDeadItems *dead_items; /* TIDs whose index tuples we'll delete */

Isn't it better to keep these comments atop the structure VacDeadItems
declaration?

2. What is the reason for not moving
lazy_vacuum_one_index/lazy_cleanup_one_index to vacuum.c so that they
can be called from vacuumlazy.c and vacuumparallel.c? Without this
refactoring patch, I think both leader and workers set the same error
context phase (VACUUM_ERRCB_PHASE_VACUUM_INDEX) during index
vacuuming? Is it because you want a separate context phase for a
parallel vacuum? The other thing related to this is that if we have to
do the way you have it here then we don't need pg_rusage_init() in
functions lazy_vacuum_one_index/lazy_cleanup_one_index.

3.
@@ -3713,7 +3152,7 @@ update_index_statistics(LVRelState *vacrel)
  int nindexes = vacrel->nindexes;
  IndexBulkDeleteResult **indstats = vacrel->indstats;

- Assert(!IsInParallelMode());
+ Assert(!ParallelVacuumIsActive(vacrel));

I think we can retain the older Assert. If we do that then I think we
don't need to define ParallelVacuumIsActive in vacuumlazy.c.

-- 
With Regards,
Amit Kapila.

  81. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-20T03:03:01Z

    On Sat, Dec 18, 2021 at 3:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Dec 17, 2021 at 10:51 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached updated patches. The first patch just moves common
> > function for index bulk-deletion and cleanup to vacuum.c. And the
> > second patch moves parallel vacuum code to vacuumparallel.c. The
> > comments I got so far are incorporated into these patches. Please
> > review them.
> >
>
> Thanks, it is helpful for the purpose of review.
>
> Few comments:
> =============
> 1.
> + * dead_items stores TIDs whose index tuples are deleted by index vacuuming.
> + * Each TID points to an LP_DEAD line pointer from a heap page that has been
> + * processed by lazy_scan_prune.  Also needed by lazy_vacuum_heap_rel, which
> + * marks the same LP_DEAD line pointers as LP_UNUSED during second heap pass.
>   */
> - LVDeadItems *dead_items; /* TIDs whose index tuples we'll delete */
> + VacDeadItems *dead_items; /* TIDs whose index tuples we'll delete */
>
> Isn't it better to keep these comments atop the structure VacDeadItems
> declaration?

I think LP_DEAD and LP_UNUSED stuff are specific to heap. Given moving
VacDeadItems to vacuum.c, I thought it's better to keep it as generic
TID storage.

>
> 2. What is the reason for not moving
> lazy_vacuum_one_index/lazy_cleanup_one_index to vacuum.c so that they
> can be called from vacuumlazy.c and vacuumparallel.c? Without this
> refactoring patch, I think both leader and workers set the same error
> context phase (VACUUM_ERRCB_PHASE_VACUUM_INDEX) during index
> vacuuming? Is it because you want a separate context phase for a
> parallel vacuum?

Since the phases defined as VacErrPhase like
VACUUM_ERRCB_PHASE_SCAN_HEAP and VACUUM_ERRCB_PHASE_VACUUM_HEAP etc.
and error callback function, vacuum_error_callback(), are specific to
heap, I thought it'd not be a good idea to move
lazy_vacuum/cleanup_one_index() so that both vacuumlazy.c and
vacuumparallel.c can use the phases and error callback function.

> The other thing related to this is that if we have to
> do the way you have it here then we don't need pg_rusage_init() in
> functions lazy_vacuum_one_index/lazy_cleanup_one_index.

Right. It should be removed.

>
> 3.
> @@ -3713,7 +3152,7 @@ update_index_statistics(LVRelState *vacrel)
>   int nindexes = vacrel->nindexes;
>   IndexBulkDeleteResult **indstats = vacrel->indstats;
>
> - Assert(!IsInParallelMode());
> + Assert(!ParallelVacuumIsActive(vacrel));
>
> I think we can retain the older Assert. If we do that then I think we
> don't need to define ParallelVacuumIsActive in vacuumlazy.c.

Right, will fix in the next version patch.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  82. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-20T04:08:36Z

    On Mon, Dec 20, 2021 at 8:33 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sat, Dec 18, 2021 at 3:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Few comments:
> > =============
> > 1.
> > + * dead_items stores TIDs whose index tuples are deleted by index vacuuming.
> > + * Each TID points to an LP_DEAD line pointer from a heap page that has been
> > + * processed by lazy_scan_prune.  Also needed by lazy_vacuum_heap_rel, which
> > + * marks the same LP_DEAD line pointers as LP_UNUSED during second heap pass.
> >   */
> > - LVDeadItems *dead_items; /* TIDs whose index tuples we'll delete */
> > + VacDeadItems *dead_items; /* TIDs whose index tuples we'll delete */
> >
> > Isn't it better to keep these comments atop the structure VacDeadItems
> > declaration?
>
> I think LP_DEAD and LP_UNUSED stuff are specific to heap. Given moving
> VacDeadItems to vacuum.c, I thought it's better to keep it as generic
> TID storage.
>

Okay, that makes sense.

> >
> > 2. What is the reason for not moving
> > lazy_vacuum_one_index/lazy_cleanup_one_index to vacuum.c so that they
> > can be called from vacuumlazy.c and vacuumparallel.c? Without this
> > refactoring patch, I think both leader and workers set the same error
> > context phase (VACUUM_ERRCB_PHASE_VACUUM_INDEX) during index
> > vacuuming? Is it because you want a separate context phase for a
> > parallel vacuum?
>
> Since the phases defined as VacErrPhase like
> VACUUM_ERRCB_PHASE_SCAN_HEAP and VACUUM_ERRCB_PHASE_VACUUM_HEAP etc.
> and error callback function, vacuum_error_callback(), are specific to
> heap, I thought it'd not be a good idea to move
> lazy_vacuum/cleanup_one_index() so that both vacuumlazy.c and
> vacuumparallel.c can use the phases and error callback function.
>

How about exposing it via heapam.h? We have already exposed a few
things via heapam.h (see /* in heap/vacuumlazy.c */). In the current
proposal, we need to have separate callbacks and phases for index
vacuuming so that it can be used by both vacuumlazy.c and
vacuumparallel.c which might not be a good idea.

-- 
With Regards,
Amit Kapila.

  83. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-20T12:59:15Z

    On Mon, Dec 20, 2021 at 1:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Dec 20, 2021 at 8:33 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Sat, Dec 18, 2021 at 3:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > Few comments:
> > > =============
> > > 1.
> > > + * dead_items stores TIDs whose index tuples are deleted by index vacuuming.
> > > + * Each TID points to an LP_DEAD line pointer from a heap page that has been
> > > + * processed by lazy_scan_prune.  Also needed by lazy_vacuum_heap_rel, which
> > > + * marks the same LP_DEAD line pointers as LP_UNUSED during second heap pass.
> > >   */
> > > - LVDeadItems *dead_items; /* TIDs whose index tuples we'll delete */
> > > + VacDeadItems *dead_items; /* TIDs whose index tuples we'll delete */
> > >
> > > Isn't it better to keep these comments atop the structure VacDeadItems
> > > declaration?
> >
> > I think LP_DEAD and LP_UNUSED stuff are specific to heap. Given moving
> > VacDeadItems to vacuum.c, I thought it's better to keep it as generic
> > TID storage.
> >
>
> Okay, that makes sense.
>
> > >
> > > 2. What is the reason for not moving
> > > lazy_vacuum_one_index/lazy_cleanup_one_index to vacuum.c so that they
> > > can be called from vacuumlazy.c and vacuumparallel.c? Without this
> > > refactoring patch, I think both leader and workers set the same error
> > > context phase (VACUUM_ERRCB_PHASE_VACUUM_INDEX) during index
> > > vacuuming? Is it because you want a separate context phase for a
> > > parallel vacuum?
> >
> > Since the phases defined as VacErrPhase like
> > VACUUM_ERRCB_PHASE_SCAN_HEAP and VACUUM_ERRCB_PHASE_VACUUM_HEAP etc.
> > and error callback function, vacuum_error_callback(), are specific to
> > heap, I thought it'd not be a good idea to move
> > lazy_vacuum/cleanup_one_index() so that both vacuumlazy.c and
> > vacuumparallel.c can use the phases and error callback function.
> >
>
> How about exposing it via heapam.h? We have already exposed a few
> things via heapam.h (see /* in heap/vacuumlazy.c */). In the current
> proposal, we need to have separate callbacks and phases for index
> vacuuming so that it can be used by both vacuumlazy.c and
> vacuumparallel.c which might not be a good idea.

Yeah, but if we expose VacErrPhase and vacuum_error_callback(), we
need to also expose LVRelState and vacuumparallel.c also uses it,
which seems not a good idea. So we will need to introduce a new struct
dedicated to the error callback function. Is that right?

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  84. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-21T03:05:27Z

    On Mon, Dec 20, 2021 at 6:29 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Dec 20, 2021 at 1:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > > >
> > > > 2. What is the reason for not moving
> > > > lazy_vacuum_one_index/lazy_cleanup_one_index to vacuum.c so that they
> > > > can be called from vacuumlazy.c and vacuumparallel.c? Without this
> > > > refactoring patch, I think both leader and workers set the same error
> > > > context phase (VACUUM_ERRCB_PHASE_VACUUM_INDEX) during index
> > > > vacuuming? Is it because you want a separate context phase for a
> > > > parallel vacuum?
> > >
> > > Since the phases defined as VacErrPhase like
> > > VACUUM_ERRCB_PHASE_SCAN_HEAP and VACUUM_ERRCB_PHASE_VACUUM_HEAP etc.
> > > and error callback function, vacuum_error_callback(), are specific to
> > > heap, I thought it'd not be a good idea to move
> > > lazy_vacuum/cleanup_one_index() so that both vacuumlazy.c and
> > > vacuumparallel.c can use the phases and error callback function.
> > >
> >
> > How about exposing it via heapam.h? We have already exposed a few
> > things via heapam.h (see /* in heap/vacuumlazy.c */). In the current
> > proposal, we need to have separate callbacks and phases for index
> > vacuuming so that it can be used by both vacuumlazy.c and
> > vacuumparallel.c which might not be a good idea.
>
> Yeah, but if we expose VacErrPhase and vacuum_error_callback(), we
> need to also expose LVRelState and vacuumparallel.c also uses it,
> which seems not a good idea. So we will need to introduce a new struct
> dedicated to the error callback function. Is that right?
>

Right, but that also doesn't sound good to me. I think it is better to
keep a separate error context for parallel vacuum workers as you have
in the patch. However, let's add some comments to indicate that if
there is a change in one of the context functions, the other should be
changed. BTW, if we go with that then we should set the correct phase
for workers as well?

-- 
With Regards,
Amit Kapila.

  85. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-21T04:34:37Z

    On Tue, Dec 21, 2021 at 12:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Dec 20, 2021 at 6:29 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Dec 20, 2021 at 1:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > > >
> > > > > 2. What is the reason for not moving
> > > > > lazy_vacuum_one_index/lazy_cleanup_one_index to vacuum.c so that they
> > > > > can be called from vacuumlazy.c and vacuumparallel.c? Without this
> > > > > refactoring patch, I think both leader and workers set the same error
> > > > > context phase (VACUUM_ERRCB_PHASE_VACUUM_INDEX) during index
> > > > > vacuuming? Is it because you want a separate context phase for a
> > > > > parallel vacuum?
> > > >
> > > > Since the phases defined as VacErrPhase like
> > > > VACUUM_ERRCB_PHASE_SCAN_HEAP and VACUUM_ERRCB_PHASE_VACUUM_HEAP etc.
> > > > and error callback function, vacuum_error_callback(), are specific to
> > > > heap, I thought it'd not be a good idea to move
> > > > lazy_vacuum/cleanup_one_index() so that both vacuumlazy.c and
> > > > vacuumparallel.c can use the phases and error callback function.
> > > >
> > >
> > > How about exposing it via heapam.h? We have already exposed a few
> > > things via heapam.h (see /* in heap/vacuumlazy.c */). In the current
> > > proposal, we need to have separate callbacks and phases for index
> > > vacuuming so that it can be used by both vacuumlazy.c and
> > > vacuumparallel.c which might not be a good idea.
> >
> > Yeah, but if we expose VacErrPhase and vacuum_error_callback(), we
> > need to also expose LVRelState and vacuumparallel.c also uses it,
> > which seems not a good idea. So we will need to introduce a new struct
> > dedicated to the error callback function. Is that right?
> >
>
> Right, but that also doesn't sound good to me.

Me too.

>  I think it is better to
> keep a separate error context for parallel vacuum workers as you have
> in the patch. However, let's add some comments to indicate that if
> there is a change in one of the context functions, the other should be
> changed.

Agreed.

>  BTW, if we go with that then we should set the correct phase
> for workers as well?

If we have separate error context for the leader (vacuumlazy.c) and
workers (vacuumparallel.c), workers don't necessarily need to have the
phases such as  VACUUM_ERRCB_PHASE_VACUUM_INDEX and
VACUUM_ERRCB_PHASE_INDEX_CLEANUP. They can use PVIndVacStatus in the
error callback function as the patch does.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  86. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-21T05:04:06Z

    On Tue, Dec 21, 2021 at 10:05 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 12:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Dec 20, 2021 at 6:29 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> >  BTW, if we go with that then we should set the correct phase
> > for workers as well?
>
> If we have separate error context for the leader (vacuumlazy.c) and
> workers (vacuumparallel.c), workers don't necessarily need to have the
> phases such as  VACUUM_ERRCB_PHASE_VACUUM_INDEX and
> VACUUM_ERRCB_PHASE_INDEX_CLEANUP. They can use PVIndVacStatus in the
> error callback function as the patch does.
>

Okay. One minor point, let's change comments atop vacuum.c considering
the movement of new functions.


-- 
With Regards,
Amit Kapila.

  87. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-21T05:53:37Z

    On Tue, Dec 21, 2021 at 2:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 10:05 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Dec 21, 2021 at 12:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Dec 20, 2021 at 6:29 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > >  BTW, if we go with that then we should set the correct phase
> > > for workers as well?
> >
> > If we have separate error context for the leader (vacuumlazy.c) and
> > workers (vacuumparallel.c), workers don't necessarily need to have the
> > phases such as  VACUUM_ERRCB_PHASE_VACUUM_INDEX and
> > VACUUM_ERRCB_PHASE_INDEX_CLEANUP. They can use PVIndVacStatus in the
> > error callback function as the patch does.
> >
>
> Okay. One minor point, let's change comments atop vacuum.c considering
> the movement of new functions.

Thank you for the comment. Agreed.

I've attached updated version patches. Please review them.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  88. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-21T13:24:03Z

    On Tue, Dec 21, 2021 at 11:24 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 2:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
>
> Thank you for the comment. Agreed.
>
> I've attached updated version patches. Please review them.
>

These look mostly good to me. Please find attached the slightly edited
version of the 0001 patch. I have modified comments, ran pgindent, and
modify the commit message. I'll commit this tomorrow if you are fine
with it.

-- 
With Regards,
Amit Kapila.

  89. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-22T00:38:27Z

    On Tue, Dec 21, 2021 at 10:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 11:24 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Dec 21, 2021 at 2:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> >
> > Thank you for the comment. Agreed.
> >
> > I've attached updated version patches. Please review them.
> >
>
> These look mostly good to me. Please find attached the slightly edited
> version of the 0001 patch. I have modified comments, ran pgindent, and
> modify the commit message. I'll commit this tomorrow if you are fine
> with it.

Thank you for the patch! It looks good to me.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  90. RE: parallel vacuum comments

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-12-22T01:14:26Z

    On Wed, Dec 22, 2021 8:38 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Tue, Dec 21, 2021 at 10:24 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Tue, Dec 21, 2021 at 11:24 AM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> > >
> > > On Tue, Dec 21, 2021 at 2:04 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > > >
> > >
> > > Thank you for the comment. Agreed.
> > >
> > > I've attached updated version patches. Please review them.
> > >
> >
> > These look mostly good to me. Please find attached the slightly edited
> > version of the 0001 patch. I have modified comments, ran pgindent, and
> > modify the commit message. I'll commit this tomorrow if you are fine
> > with it.
> 
> It looks good to me.
+1, the patch passes check-world and looks good to me.

Best regards,
Hou zj

  91. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-22T03:35:30Z

    On Tue, Dec 21, 2021 at 10:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 11:24 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Dec 21, 2021 at 2:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> >
> > Thank you for the comment. Agreed.
> >
> > I've attached updated version patches. Please review them.
> >
>
> These look mostly good to me. Please find attached the slightly edited
> version of the 0001 patch. I have modified comments, ran pgindent, and
> modify the commit message. I'll commit this tomorrow if you are fine
> with it.

Thank you for committing the first patch.

I've attached an updated version second patch. Please review it.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  92. RE: parallel vacuum comments

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-12-22T12:09:23Z

    On Wed, Dec 22, 2021 11:36 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Tue, Dec 21, 2021 at 10:24 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Tue, Dec 21, 2021 at 11:24 AM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> > >
> > > On Tue, Dec 21, 2021 at 2:04 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > > >
> > >
> > > Thank you for the comment. Agreed.
> > >
> > > I've attached updated version patches. Please review them.
> > >
> >
> > These look mostly good to me. Please find attached the slightly edited
> > version of the 0001 patch. I have modified comments, ran pgindent, and
> > modify the commit message. I'll commit this tomorrow if you are fine
> > with it.
> 
> Thank you for committing the first patch.
> 
> I've attached an updated version second patch. Please review it.
> 
> Regards,

Hi,

The patch looks mostly good to me.
I only have few comments.

1)
+/*
+ * Do parallel index bulk-deletion with parallel workers.
+ */
+void
+parallel_vacuum_bulkdel_all_indexes(ParallelVacuumState *pvs, long num_table_tuples)
+{
+	Assert(!IsParallelWorker());
+

Would it be better to also put Assert(pvs != NULL) here ? Because we removed
the Assert(ParallelVacuumIsActive(vacrel)) check in the old function.


2)
+#include "utils/rel.h"
+#include "utils/lsyscache.h"
+#include "utils/memutils.h"

It might be better to keep the header file in alphabetical order.
:
+#include "utils/lsyscache.h"
+#include "utils/memutils.h"
+#include "utils/rel.h"

Best regards,
Hou zj

  93. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-22T12:52:01Z

    On Wed, Dec 22, 2021 at 5:39 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Wed, Dec 22, 2021 11:36 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > On Tue, Dec 21, 2021 at 10:24 PM Amit Kapila <amit.kapila16@gmail.com>
> > wrote:
>
> The patch looks mostly good to me.
> I only have few comments.
>
> 1)
> +/*
> + * Do parallel index bulk-deletion with parallel workers.
> + */
> +void
> +parallel_vacuum_bulkdel_all_indexes(ParallelVacuumState *pvs, long num_table_tuples)
> +{
> +       Assert(!IsParallelWorker());
> +
>
> Would it be better to also put Assert(pvs != NULL) here ? Because we removed
> the Assert(ParallelVacuumIsActive(vacrel)) check in the old function.
>

I am not sure if that is helpful or not because there is only one
caller of it which checks pvs before calling this function.

>
> 2)
> +#include "utils/rel.h"
> +#include "utils/lsyscache.h"
> +#include "utils/memutils.h"
>
> It might be better to keep the header file in alphabetical order.
> :
> +#include "utils/lsyscache.h"
> +#include "utils/memutils.h"
> +#include "utils/rel.h"
>

Right, I'll take care of this as I am already making some other edits
in the patch.

-- 
With Regards,
Amit Kapila.

  94. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-22T13:55:12Z

    On Wed, Dec 22, 2021 at 6:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Dec 22, 2021 at 5:39 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> >
> > 2)
> > +#include "utils/rel.h"
> > +#include "utils/lsyscache.h"
> > +#include "utils/memutils.h"
> >
> > It might be better to keep the header file in alphabetical order.
> > :
> > +#include "utils/lsyscache.h"
> > +#include "utils/memutils.h"
> > +#include "utils/rel.h"
> >
>
> Right, I'll take care of this as I am already making some other edits
> in the patch.
>

Fixed this and made a few other changes in the patch that includes (a)
passed down the num_index_scans information in parallel APIs based on
which it can make the decision whether to reinitialize DSM or consider
conditional parallel vacuum clean up; (b) get rid of first-time
variable in ParallelVacuumState as that is not required if we have
num_index_scans information; (c) there seems to be quite a few
unnecessary includes in vacuumparallel.c which I have removed; (d)
unnecessary error callback info was being set in ParallelVacuumState
in leader backend; (e) changed/added comments at quite a few places.

Can you please once verify the changes in the attached?

-- 
With Regards,
Amit Kapila.

  95. RE: parallel vacuum comments

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-12-23T02:38:31Z

    On Wed, Dec 22, 2021 9:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Dec 22, 2021 at 6:22 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Wed, Dec 22, 2021 at 5:39 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > >
> > > 2)
> > > +#include "utils/rel.h"
> > > +#include "utils/lsyscache.h"
> > > +#include "utils/memutils.h"
> > >
> > > It might be better to keep the header file in alphabetical order.
> > > :
> > > +#include "utils/lsyscache.h"
> > > +#include "utils/memutils.h"
> > > +#include "utils/rel.h"
> > >
> >
> > Right, I'll take care of this as I am already making some other edits
> > in the patch.
> >
> 
> Fixed this and made a few other changes in the patch that includes (a) passed
> down the num_index_scans information in parallel APIs based on which it can
> make the decision whether to reinitialize DSM or consider conditional parallel
> vacuum clean up; (b) get rid of first-time variable in ParallelVacuumState as that
> is not required if we have num_index_scans information; (c) there seems to be
> quite a few unnecessary includes in vacuumparallel.c which I have removed; (d)
> unnecessary error callback info was being set in ParallelVacuumState in leader
> backend; (e) changed/added comments at quite a few places.
> 
> Can you please once verify the changes in the attached?

The changes look ok to me.
I tested the patch for multi-pass parallel vacuum cases and ran 'make check-world',
all the tests passed

Best regards,
Hou zj

  96. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-23T05:26:03Z

    On Wed, Dec 22, 2021 at 10:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Dec 22, 2021 at 6:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Dec 22, 2021 at 5:39 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > >
> > > 2)
> > > +#include "utils/rel.h"
> > > +#include "utils/lsyscache.h"
> > > +#include "utils/memutils.h"
> > >
> > > It might be better to keep the header file in alphabetical order.
> > > :
> > > +#include "utils/lsyscache.h"
> > > +#include "utils/memutils.h"
> > > +#include "utils/rel.h"
> > >
> >
> > Right, I'll take care of this as I am already making some other edits
> > in the patch.
> >
>
> Fixed this and made a few other changes in the patch that includes (a)
> passed down the num_index_scans information in parallel APIs based on
> which it can make the decision whether to reinitialize DSM or consider
> conditional parallel vacuum clean up; (b) get rid of first-time
> variable in ParallelVacuumState as that is not required if we have
> num_index_scans information; (c) there seems to be quite a few
> unnecessary includes in vacuumparallel.c which I have removed; (d)
> unnecessary error callback info was being set in ParallelVacuumState
> in leader backend; (e) changed/added comments at quite a few places.
>
> Can you please once verify the changes in the attached?

Thank you for updating the patch!

I agreed with these changes and it looks good to me.


Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

  97. Re: parallel vacuum comments

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-24T02:59:22Z

    On Thu, Dec 23, 2021 at 10:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Dec 22, 2021 at 10:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Dec 22, 2021 at 6:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Dec 22, 2021 at 5:39 PM houzj.fnst@fujitsu.com
> > > <houzj.fnst@fujitsu.com> wrote:
> > > >
> > > >
> > > > 2)
> > > > +#include "utils/rel.h"
> > > > +#include "utils/lsyscache.h"
> > > > +#include "utils/memutils.h"
> > > >
> > > > It might be better to keep the header file in alphabetical order.
> > > > :
> > > > +#include "utils/lsyscache.h"
> > > > +#include "utils/memutils.h"
> > > > +#include "utils/rel.h"
> > > >
> > >
> > > Right, I'll take care of this as I am already making some other edits
> > > in the patch.
> > >
> >
> > Fixed this and made a few other changes in the patch that includes (a)
> > passed down the num_index_scans information in parallel APIs based on
> > which it can make the decision whether to reinitialize DSM or consider
> > conditional parallel vacuum clean up; (b) get rid of first-time
> > variable in ParallelVacuumState as that is not required if we have
> > num_index_scans information; (c) there seems to be quite a few
> > unnecessary includes in vacuumparallel.c which I have removed; (d)
> > unnecessary error callback info was being set in ParallelVacuumState
> > in leader backend; (e) changed/added comments at quite a few places.
> >
> > Can you please once verify the changes in the attached?
>
> Thank you for updating the patch!
>
> I agreed with these changes and it looks good to me.
>

Pushed. As per my knowledge, we have addressed the improvements
raised/discussed in this thread.

-- 
With Regards,
Amit Kapila.

  98. Re: parallel vacuum comments

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-12-24T03:15:06Z

    On Fri, Dec 24, 2021 at 11:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Dec 23, 2021 at 10:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Dec 22, 2021 at 10:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Dec 22, 2021 at 6:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Wed, Dec 22, 2021 at 5:39 PM houzj.fnst@fujitsu.com
> > > > <houzj.fnst@fujitsu.com> wrote:
> > > > >
> > > > >
> > > > > 2)
> > > > > +#include "utils/rel.h"
> > > > > +#include "utils/lsyscache.h"
> > > > > +#include "utils/memutils.h"
> > > > >
> > > > > It might be better to keep the header file in alphabetical order.
> > > > > :
> > > > > +#include "utils/lsyscache.h"
> > > > > +#include "utils/memutils.h"
> > > > > +#include "utils/rel.h"
> > > > >
> > > >
> > > > Right, I'll take care of this as I am already making some other edits
> > > > in the patch.
> > > >
> > >
> > > Fixed this and made a few other changes in the patch that includes (a)
> > > passed down the num_index_scans information in parallel APIs based on
> > > which it can make the decision whether to reinitialize DSM or consider
> > > conditional parallel vacuum clean up; (b) get rid of first-time
> > > variable in ParallelVacuumState as that is not required if we have
> > > num_index_scans information; (c) there seems to be quite a few
> > > unnecessary includes in vacuumparallel.c which I have removed; (d)
> > > unnecessary error callback info was being set in ParallelVacuumState
> > > in leader backend; (e) changed/added comments at quite a few places.
> > >
> > > Can you please once verify the changes in the attached?
> >
> > Thank you for updating the patch!
> >
> > I agreed with these changes and it looks good to me.
> >
>
> Pushed.

Thank you for committing the patch!

> As per my knowledge, we have addressed the improvements
> raised/discussed in this thread.

Yeah, I think so too.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/