The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Hello
I reviewed v25 patches and have just a few notes.
missed synopsis for "PARALLEL" option (<synopsis> block in
doc/src/sgml/ref/vacuum.sgml )
missed prototype for vacuum_log_cleanup_info in "non-export function prototypes"
> /*
> * Do post-vacuum cleanup, and statistics update for each index if
> * we're not in parallel lazy vacuum. If in parallel lazy vacuum, do
> * only post-vacum cleanup and update statistics at the end of parallel
> * lazy vacuum.
> */
> if (vacrelstats->useindex)
> lazy_vacuum_or_cleanup_indexes(vacrelstats, Irel, nindexes,
>
> indstats, lps, true);
>
> if (ParallelVacuumIsActive(lps))
> {
> /* End parallel mode and update index statistics */
> end_parallel_vacuum(lps, Irel, nindexes);
> }
I personally do not like update statistics in different places.
Can we change lazy_vacuum_or_cleanup_indexes to writing stats for both parallel
and non-parallel cases? I means something like this:
> if (ParallelVacuumIsActive(lps))
> {
> /* Do parallel index vacuuming or index cleanup */
> lazy_parallel_vacuum_or_cleanup_indexes(vacrelstats, Irel,
>
> nindexes, stats,
>
> lps, for_cleanup);
> if (for_cleanup)
> {
> ...
> for (i = 0; i < nindexes; i++)
> lazy_update_index_statistics(...);
> }
> return;
> }
So all lazy_update_index_statistics would be in one place.
lazy_parallel_vacuum_or_cleanup_indexes is called only from parallel leader and
waits for all workers. Possible we can update stats in
lazy_parallel_vacuum_or_cleanup_indexes after WaitForParallelWorkersToFinish
call.
Also discussion question: vacuumdb parameters --parallel= and --jobs= will
confuse users? We need more description for this options?
regards, Sergei
