Re: Unifying VACUUM VERBOSE and log_autovacuum_min_duration output

On Tue, Dec 21, 2021 at 11:57 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've looked at the patch and here are comments:

Thanks!

The patch bitrot again, so attached is a rebased version, v3.

> I think we should set message_level. Otherwise, index AM will set an
> invalid log level, although any index AM in core seems not to use it.

Fixed.

> ---
> -               /*
> -                * Update error traceback information.  This is the
> last phase during
> -                * which we add context information to errors, so we
> don't need to
> -                * revert to the previous phase.
> -                */
>
> Why is this comment removed? ISTM this comment is still valid.

We don't "revert to the previous phase" here, which is always
VACUUM_ERRCB_PHASE_SCAN_HEAP in practice, per the comment -- but that
doesn't seem significant. It's not just unnecessary to do so, as the
comment claims -- it actually seems *wrong*. That is, it would be
wrong to go back to VACUUM_ERRCB_PHASE_SCAN_HEAP here, since we're
completely finished scanning the heap at this point.

There is still perhaps a small danger that somebody will forget to add
a new VACUUM_ERRCB_PHASE_* for some new kind of work that happens
after this point, at the very last moment. But that would be equally
true if the new kind of work took place earlier, inside
lazy_scan_heap(). And so the last call to update_vacuum_error_info()
isn't special compared to any other update_vacuum_error_info() call
(or any other call that doesn't set a saved_err_info).

--
Peter Geoghegan