Re: POC: Parallel processing of indexes in autovacuum

Hi,

I took a look at v20-0001,0002 and 0003 and have some comments.

v20-0001:

1/

```
+
+ /*
+ * Cap or increase number of free parallel workers according to the
+ * parameter change.
+ */
+ AutoVacuumShmem->av_freeParallelWorkers = Max(nfree_workers, 0);
+
+ /*
+ * Don't allow number of free workers to become less than zero if the
+ * patameter was decreased.
+ */
+ AutoVacuumShmem->av_freeParallelWorkers =
+ Max(AutoVacuumShmem->av_freeParallelWorkers, 0);
```

This can probably be simplified to:

```
AutoVacuumShmem->av_freeParallelWorkers = Max(nfree_workers, 0);
```

v20-0002:

1/

I don't think showing "reserved" in the logging is needed and could be
confusing.

```
parallel index vacuum/cleanup: 3 workers were planned, 3 workers were
reserved and 3 workers were launched in total
```

Also, even though the table has `autovacuum_parallel_workers = 2`, I
see 3 workers.
This is because one worker was for cleanup due to a gin index on the
table. I think it's better
to show separate lines for index vacuuming and index cleanup, just
like VACUUM VERBOSE.

```
INFO: launched 2 parallel vacuum workers for index vacuuming (planned: 2)
INFO: launched 1 parallel vacuum worker for index cleanup (planned: 1)
```

otherwise it will lead the user to think 3 workers were allocated for
either vacuuming or cleanup.

v20-0003:

1/

inside vacuum_delay_point, I would re-organize the checks to
first run the code block for the a/v worker:

```
if (ConfigReloadPending && AmAutoVacuumWorkerProcess())
```

then the a/v/ parallel worker:

```
if (!AmAutoVacuumWorkerProcess() && IsParallelWorker())
```

But I am also wondering if we should have a specific backend_type
for "autovacuum parallel worker" to differentiate that from the
existing "autovacuum worker".

and also we can have a helper macro like:
```
#define AmAutoVacuumParallelWorkerProcess() (MyBackendType ==
B_AUTOVAC_PARALLEL_WORKER)
```

What do you think?

2/

Add
```
+typedef struct PVSharedCostParams
````

to src/tools/pgindent/typedefs.list

3/

+ pg_atomic_init_u32(&shared->cost_params.generation, 0);
+ SpinLockInit(&shared->cost_params.spinlock);
+ pv_shared_cost_params = &(shared->cost_params);

NIT: move SpinLockInit last

4/

Instead of:

```
+ params_generation =
pg_atomic_read_u32(&pv_shared_cost_params->generation);
+
```
and then later on:
````
+ /*
+ * Increase generation of the parameters, i.e. let parallel workers know
+ * that they should re-read shared cost params.
+ */
+ pg_atomic_write_u32(&pv_shared_cost_params->generation,
+ params_generation + 1);
+
+ SpinLockRelease(&pv_shared_cost_params->spinlock);
```

why can't we just do:

pg_atomic_fetch_add_u32(&pv_shared_cost_params->generation, 1);

Also, do the pg_atomic_fetch_add_u32 outside of the spinlock. right?

--
Sami Imseih
Amazon Web Services (AWS)