On Wed, 15 Jan 2020 at 19:31, Mahendra Singh Thalor <mahi6...@gmail.com> wrote: > > On Wed, 15 Jan 2020 at 19:04, Mahendra Singh Thalor <mahi6...@gmail.com> > wrote: > > > > On Wed, 15 Jan 2020 at 17:27, Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Wed, Jan 15, 2020 at 10:05 AM Masahiko Sawada > > > <masahiko.saw...@2ndquadrant.com> wrote: > > > > > > > > Thank you for updating the patch! I have a few small comments. > > > > > > > > > > I have adapted all your changes, fixed the comment by Mahendra related > > > to initializing parallel state only when there are at least two > > > indexes. Additionally, I have changed a few comments (make the > > > reference to parallel vacuum consistent, at some places we were > > > referring it as 'parallel lazy vacuum' and at other places it was > > > 'parallel index vacuum'). > > > > > > > The > > > > rest looks good to me. > > > > > > > > > > Okay, I think the patch is in good shape. I am planning to read it a > > > few more times (at least 2 times) and then probably will commit it > > > early next week (Monday or Tuesday) unless there are any major > > > comments. I have already committed the API patch (4d8a8d0c73). > > > > > > > Hi, > > Thanks Amit for fixing review comments. > > > > I reviewed v48 patch and below are some comments. > > > > 1. > > + * based on the number of indexes. -1 indicates a parallel vacuum is > > > > I think, above should be like "-1 indicates that parallel vacuum is" > > > > 2. > > +/* Variables for cost-based parallel vacuum */ > > > > At the end of comment, there is 2 spaces. I think, it should be only 1 > > space. > > > > 3. > > I think, we should add a test case for parallel option(when degree is not > > specified). > > Ex: > > postgres=# VACUUM (PARALLEL) tmp; > > ERROR: parallel option requires a value between 0 and 1024 > > LINE 1: VACUUM (PARALLEL) tmp; > > ^ > > postgres=# > > > > Because above error is added in this parallel patch, so we should have test > > case for this to increase code coverage. > > > > Hi > Below are some more review comments for v48 patch. > > 1. > #include "storage/bufpage.h" > #include "storage/lockdefs.h" > +#include "storage/shm_toc.h" > +#include "storage/dsm.h" > > Here, order of header file is not alphabetically. (storage/dsm.h > should come before storage/lockdefs.h) > > 2. > + /* No index supports parallel vacuum */ > + if (nindexes_parallel == 0) > + return 0; > + > + /* The leader process takes one index */ > + nindexes_parallel--; > > Above code can be rearranged as: > > + /* The leader process takes one index */ > + nindexes_parallel--; > + > + /* No index supports parallel vacuum */ > + if (nindexes_parallel <= 0) > + return 0; > > If we do like this, then in some cases, we can skip some calculations > of parallel workers. > > -- > Thanks and Regards > Mahendra Singh Thalor > EnterpriseDB: http://www.enterprisedb.com
Hi, I checked code coverage and time taken by vacuum.sql test with and without v48 patch. Below are some findings (I ran "make check-world -i" to get coverage.) 1. With v45 patch, compute_parallel_delay is never called so function hit is zero. I think, we can add some delay options into vacuum.sql test to hit function. 2. I checked time taken by vacuum.sql test. Execution time is almost same with and without v45 patch. Without v45 patch: Run1) vacuum ... ok 701 ms Run2) vacuum ... ok 549 ms Run3) vacuum ... ok 559 ms Run4) vacuum ... ok 480 ms With v45 patch: Run1) vacuum ... ok 842 ms Run2) vacuum ... ok 808 ms Run3) vacuum ... ok 774 ms Run4) vacuum ... ok 792 ms -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
