Обсуждение: parallel vacuum comments
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
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
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/
Вложения
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
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/
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.
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/
Вложения
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
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/
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.
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/
Вложения
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
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
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
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
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/
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/
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/
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/
Вложения
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.
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/
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.
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/
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
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/
Вложения
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
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.
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.
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/
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/
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/
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.
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.
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.
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/
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/
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.
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/
Вложения
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
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.
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/
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/
Вложения
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.
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
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.
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.
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/
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/
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.
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/
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.
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/
Вложения
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
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/
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/
Вложения
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.
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.
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/
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/
Вложения
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.
Вложения
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
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/
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.
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/
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/
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.
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/
Вложения
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
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
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.
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
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.
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/
Вложения
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.
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
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/
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.
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/
Вложения
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/
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.
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/
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.
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/
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.
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/
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.
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.
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/