Re: [HACKERS] Block level parallel vacuum

On Wed, 27 Nov 2019 at 13:28, Mahendra Singh <mahi6run(at)gmail(dot)com> wrote:
>
> 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.

Thank you for testing!

Yes, I think your analysis is right. I've removed the assertion in v35
patch that I've just posted[1].

[1] https://www.postgresql.org/message-id/CA%2Bfd4k5oAuGuwZ9XaOTv%2BcTU8-dmA3RjpJ%2Bi4x5kt9VbAFse1w%40mail.gmail.com

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services