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. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
