On Wed, 27 Nov 2019 at 08:14, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Wed, Nov 27, 2019 at 12:52 AM Masahiko Sawada > <masahiko.saw...@2ndquadrant.com> wrote: > > > > > > I've incorporated the comments I got so far including the above and > > the memory alignment issue. > > > > Thanks, I will look into the new version. BTW, why haven't you posted > 0001 patch (IndexAM API's patch)? I think without that we need to use > the previous version for that. Also, I think we should post Dilip's > patch related to Gist index [1] modifications for parallel vacuum or > at least have a mention for that while posting a new version as > without that even make check fails. > > [1] - > https://www.postgresql.org/message-id/CAFiTN-uQY%2BB%2BCLb8W3YYdb7XmB9hyYFXkAy3C7RY%3D-YSWRV1DA%40mail.gmail.com > > I did some testing on the top of v33 patch set. By debugging, I was able to hit one assert in lazy_parallel_vacuum_or_cleanup_indexes. TRAP: FailedAssertion("nprocessed == nindexes_remains", File: "vacuumlazy.c", Line: 2099) I further debugged and found that this assert is not valid in all the cases. Here, nprocessed can be less than nindexes_remains in some cases because it is possible that parallel worker is launched for vacuum and idx count is incremented in vacuum_or_cleanup_indexes_worker for particular index but work is still not finished(lvshared->nprocessed is not incremented yet) so in that case, nprocessed will be less than nindexes_remains. I think, we should remove this assert. I have one comment for assert used variable: +#ifdef USE_ASSERT_CHECKING + int nprocessed = 0; +#endif I think, we can make above declaration as " int nprocessed PG_USED_FOR_ASSERTS_ONLY = 0" so that code looks good because this USE_ASSERT_CHECKING is used in 3 places in 20-30 code lines. Thanks and Regards Mahendra Thalor EnterpriseDB: http://www.enterprisedb.com
