On Thu, 16 Jan 2020 at 08:22, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Jan 16, 2020 at 1:02 AM Mahendra Singh Thalor > <mahi6...@gmail.com> wrote: > > > > 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: > > > > > > > > > > > > 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" > > > > > > I am not an expert in this matter, but I am not sure if your > suggestion is correct. I thought an article is required here, but I > could be wrong. Can you please clarify? > > > > > 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. > > > > > > I thought about it but was not sure to add a test for it. We might > not want to add a test for each and every case as that will increase > the number and time of tests without a significant advantage. Now > that you have pointed this, I can add a test for it unless someone > else thinks otherwise. > > > > > 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. > > > > But how can we meaningfully test the functionality of the delay? It > would be tricky to come up with a portable test that can always > produce consistent results. > > > 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 > > > > I see some variance in results, have you run with autovacuum as off. > I was expecting that this might speed up some cases where parallel > vacuum is used by default.
I think, this is expected difference in timing because we are adding some vacuum related test. I am not starting server manually(means I am starting server with only default setting). If we start server with default settings, then we will not hit vacuum related test cases to parallel because size of index relation is very small so we will not do parallel vacuum. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
