Re: [HACKERS] Block level parallel vacuum

On Tue, Jan 15, 2019 at 6:00 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:

>
> Rebased.
>

I started reviewing the patch, I didn't finish my review yet.
Following are some of the comments.

+ <term><literal>PARALLEL <replaceable
class="parameter">N</replaceable></literal></term>
+ <listitem>
+ <para>
+ Execute index vacuum and cleanup index in parallel with

I doubt that user can understand the terms index vacuum and cleanup index.
May be it needs some more detailed information.

- VACOPT_DISABLE_PAGE_SKIPPING = 1 << 7 /* don't skip any pages */
+ VACOPT_PARALLEL = 1 << 7, /* do lazy VACUUM in parallel */
+ VACOPT_DISABLE_PAGE_SKIPPING = 1 << 8 /* don't skip any pages */
+} VacuumOptionFlag;

Any specific reason behind not adding it as last member of the enum?

-typedef enum VacuumOption
+typedef enum VacuumOptionFlag
{

I don't find the new name quite good, how about VacuumFlags?

+typedef struct VacuumOption
+{

How about VacuumOptions? Because this structure can contains all the
options provided to vacuum operation.

+ vacopt1->flags |= vacopt2->flags;
+ if (vacopt2->flags == VACOPT_PARALLEL)
+ vacopt1->nworkers = vacopt2->nworkers;
+ pfree(vacopt2);
+ $$ = vacopt1;
+ }

As the above statement indicates the the last parallel number of workers
is considered into the account, can we explain it in docs?

postgres=# vacuum (parallel 2, verbose) tbl;

With verbose, no parallel workers related information is available.
I feel giving that information is required even when it is not parallel
vacuum also.

Regards,
Haribabu Kommi
Fujitsu Australia