Re: New IndexAM API controlling index vacuum strategies

On Mon, Apr 5, 2021 at 4:30 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> Did you try the change around parallel_process_one_index() that I
> suggested in the previous reply[1]? If we don't change the logic, we
> need to update the above comment. Previously, we update stats[idx] in
> vacuum_one_index() (renamed to parallel_process_one_index()) but with
> your patch, where we update it is its caller.

I don't know how I missed it the first time. I agree that it is a lot
better that way.

I did it that way in the version of the patch that I pushed just now. Thanks!

Do you think that it's okay that we rely on the propagation of global
state to parallel workers on Postgres 13? Don't we need something like
my fixup commit 49f49def on Postgres 13 as well? At least for the
EXEC_BACKEND case, I think.

> We removed two Assert(!IsParallelWorker()) at two places. It seems to
> me that those assertions are still valid. Do we really need to remove
> them?

I have restored the assertions in what became the final version.

> 0004 patch:
>
> src/backend/access/heap/heapam.c:638: trailing whitespace.

Will fix.

> ---
> 0005 patch:
>
> + * Caller is expected to call here before and after vacuuming each index in
> + * the case of two-pass VACUUM, or every BYPASS_EMERGENCY_MIN_PAGES blocks in
> + * the case of no-indexes/one-pass VACUUM.
>
> I think it should be "every VACUUM_FSM_EVERY_PAGES blocks" instead of
> "every BYPASS_EMERGENCY_MIN_PAGES blocks".

Will fix.

> +#define BYPASS_EMERGENCY_MIN_PAGES \
> +   ((BlockNumber) (((uint64) 4 * 1024 * 1024 * 1024) / BLCKSZ))
> +
>
> I think we need a description for BYPASS_EMERGENCY_MIN_PAGES.

I agree - will fix.

> allindexes can be false even if we process all indexes, which is fine
> with me because setting allindexes = false disables the subsequent
> heap vacuuming. I think it's appropriate behavior in emergency cases.
> In that sense, can we do should_speedup_failsafe() check also after
> parallel index vacuuming? And we can also check it at the beginning of
> lazy vacuum.

Those both seem like good ideas. Especially the one about checking
right at the start. Now that the patch makes the emergency mechanism
not apply a delay (not just skip index vacuuming), having a precheck
at the very start makes a lot of sense. This also makes VACUUM hurry
in the case where there was a dangerously slow VACUUM that happened to
not be aggressive. Such a VACUUM will use the emergency mechanism but
won't advance relfrozenxid, because we have to rely on the autovacuum
launcher launching an anti-wraparound/aggressive autovacuum
immediately afterwards. We want that second anti-wraparound VACUUM to
hurry from the very start of lazy_scan_heap().

--
Peter Geoghegan