Re: [HACKERS] Block level parallel vacuum

On Wed, 27 Nov 2019 at 08:14, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Wed, Nov 27, 2019 at 12:52 AM Masahiko Sawada
> <masahiko(dot)sawada(at)2ndquadrant(dot)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