On Tue, 14 Jan 2020 at 21:43, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Jan 14, 2020 at 10:04 AM Masahiko Sawada > <masahiko.saw...@2ndquadrant.com> wrote: > > > > On Mon, 13 Jan 2020 at 12:50, Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Sat, Jan 11, 2020 at 7:48 PM Masahiko Sawada > > > <masahiko.saw...@2ndquadrant.com> wrote: > > > > > > Okay, would it better if we get rid of this variable and have code like > > > below? > > > > > > /* Skip the indexes that can be processed by parallel workers */ > > > if ( !(get_indstats(lps->lvshared, i) == NULL || > > > skip_parallel_vacuum_index(Irel[i], lps->lvshared))) > > > continue; > > > > Make sense to me. > > > > I have changed the comment and condition to make it a positive test so > that it is more clear. > > > > ... > > > > Agreed. But with the updated patch the PARALLEL option without the > > > > parallel degree doesn't display warning because params->nworkers = 0 > > > > in that case. So how about restoring params->nworkers at the end of > > > > vacuum_rel()? > > > > > > > > > > I had also thought on those lines, but I was not entirely sure about > > > this resetting of workers. Today, again thinking about it, it seems > > > the idea Mahendra is suggesting that is giving an error if the > > > parallel degree is not specified seems reasonable to me. This means > > > Vacuum (parallel), Vacuum (parallel) <tbl_name>, etc. will give an > > > error "parallel degree must be specified". This idea has merit as now > > > we are supporting a parallel vacuum by default, so a 'parallel' option > > > without a parallel degree doesn't have any meaning. If we do that, > > > then we don't need to do anything additional about the handling of > > > temp tables (other than what patch is already doing) as well. What do > > > you think? > > > > > > > Good point! Agreed. > > > > Thanks, changed accordingly. >
Thank you for updating the patch! I have a few small comments. The
rest looks good to me.
1.
+ * Compute the number of parallel worker processes to request. Both index
+ * vacuum and index cleanup can be executed with parallel workers. The
+ * relation size of the table don't affect the parallel degree for now.
s/don't/doesn't/
2.
@@ -383,6 +435,7 @@ vacuum(List *relations, VacuumParams *params,
VacuumPageHit = 0;
VacuumPageMiss = 0;
VacuumPageDirty = 0;
+ VacuumSharedCostBalance = NULL;
I think we can initialize VacuumCostBalanceLocal and
VacuumActiveNWorkers here. We use these parameters during parallel
index vacuum and reset at the end but we might want to initialize them
for safety.
3.
+ /* Set cost-based vacuum delay */
+ VacuumCostActive = (VacuumCostDelay > 0);
+ VacuumCostBalance = 0;
+ VacuumPageHit = 0;
+ VacuumPageMiss = 0;
+ VacuumPageDirty = 0;
+ VacuumSharedCostBalance = &(lvshared->cost_balance);
+ VacuumActiveNWorkers = &(lvshared->active_nworkers);
VacuumCostBalanceLocal also needs to be initialized.
4.
The regression tests don't have the test case of PARALLEL 0.
Since I guess you already modifies the code locally I've attached the
diff containing the above review comments.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
review_v47_masahiko.patch
Description: Binary data
