Re: parallel vacuum comments

On Thu, Nov 11, 2021 at 6:38 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Nov 11, 2021 at 8:11 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > I've attached a draft patch that refactors parallel vacuum and
> > separates parallel-vacuum-related code to new file vacuumparallel.c.
> > After discussion, I'll divide the patch into logical chunks.
> >
> > What I'm not convinced yet in this patch is that vacuum.c,
> > vacuumlazy.c and vacuumparallel.c depend on the data structure to
> > store dead tuples (now called VacDeadTuples, was LVDeadTuples). I
> > thought that it might be better to separate it so that a table AM can
> > use another type of data structure to store dead tuples. But since I
> > think it may bring complexity, currently a table AM has to use
> > VacDeadTuples in order to use the parallel vacuum.
> >
>
> I think it might be better to attempt doing anything to make it
> generic for tableAM in a separate patch if that is required.

You mean to refactor relation_vacuum table AM API too? Currently,
relation_vacuum API is responsible for whole vacuum operation and
there is no room for the core doing anything during vacuum. So
probably it doesn’t make sense to have a table AM API for parallel
vacuum.

>
> Few questions/comments:
> =====================
> 1. There are three different structures PVState,
> ParallelVacuumContext, and ParallelVacuumCtl to maintain the parallel
> vacuum state. Can't we merge PVState and ParallelVacuumCtl? Also, I
> see that most of the fields of PVState are there in
> ParallelVacuumContext except for error info fields, does it makes
> sense to directly use PVState instead?

Agreed.

> Also, it would be better to
> write some more comments atop each structure to explain its usage.

Agreed.

>
> 2. In vacuum.c, the function names doesn't match the names in their
> corresponding function header comments.

Will fix.

>
> 3.
> + INDVAC_STATUS_COMPLETED,
> +} PVIndVacStatus;
>
> The comma is not required after the last value of enum.

Will fix.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/