Re: error context for vacuum to include block number

On Tue, 24 Mar 2020 at 22:37, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.

I got the point. But if we set the error context before that, I think
we need to change the error context message. The error context message
of heap truncation phase is "while truncating relation \"%s.%s\" to %u
blocks", but cbarg->blkno will be the number of blocks of the current
relation.

       case VACUUM_ERRCB_PHASE_TRUNCATE:
           if (BlockNumberIsValid(cbarg->blkno))
               errcontext("while truncating relation \"%s.%s\" to %u blocks",
                          cbarg->relnamespace, cbarg->relname, cbarg->blkno);
           break;

>
> > 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.

Right, thank you for explanation. I was concerned memory leak because
relation name and schema name could be large compared to vacrelstats
but I agree to free them at commit time.

>
> > 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.

Yeah, me too. The most likely place where an error happens is
vac_update_relstats but the error message seems to be enough.

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services