Re: error context for vacuum to include block number - Mailing list pgsql-hackers

On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
>
> I've read through the latest version patch (v31), here are my comments:
>
> 1.
> +        /* Update error traceback information */
> +        olderrcbarg = *vacrelstats;
> +        update_vacuum_error_cbarg(vacrelstats,
> +                                  VACUUM_ERRCB_PHASE_TRUNCATE,
> new_rel_pages, NULL,
> +                                  false);
> +
>          /*
>           * Scan backwards from the end to verify that the end pages actually
>           * contain no tuples.  This is *necessary*, not optional, because
>           * other backends could have added tuples to these pages whilst we
>           * were vacuuming.
>           */
>          new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
>
> We need to set the error context after setting new_rel_pages.
>

We want to cover the errors raised in count_nondeletable_pages().  In
an earlier version of the patch, we had TRUNCATE_PREFETCH phase which
use to cover those errors, but that was not good as we were
setting/resetting it multiple times and it was not clear such a
separate phase would add any value.

> 2.
> +   vacrelstats->relnamespace =
> get_namespace_name(RelationGetNamespace(onerel));
> +   vacrelstats->relname = pstrdup(RelationGetRelationName(onerel));
>
> I think we can pfree these two variables to avoid a memory leak during
> vacuum on multiple relations.
>

Yeah, I had also thought about it but I noticed that we are not
freeing for vacrelstats.  Also, I think the memory is allocated in
TopTransactionContext which should be freed via
CommitTransactionCommand before vacuuming of the next relation, so not
sure if there is much value in freeing those variables.

> 3.
> +/* Phases of vacuum during which we report error context. */
> +typedef enum
> +{
> +   VACUUM_ERRCB_PHASE_UNKNOWN,
> +   VACUUM_ERRCB_PHASE_SCAN_HEAP,
> +   VACUUM_ERRCB_PHASE_VACUUM_INDEX,
> +   VACUUM_ERRCB_PHASE_VACUUM_HEAP,
> +   VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
> +   VACUUM_ERRCB_PHASE_TRUNCATE
> +} ErrCbPhase;
>
> Comparing to the vacuum progress phases, there is not a phase for
> final cleanup which includes updating the relation stats. Is there any
> reason why we don't have that phase for ErrCbPhase?
>

I think we can add it if we want, but it was not clear to me if that is helpful.

> The type name ErrCbPhase seems to be very generic name, how about
> VacErrCbPhase or VacuumErrCbPhase?
>

It sounds like a better name.



-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com