Re: error context for vacuum to include block number

On Tue, Mar 24, 2020 at 7:51 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> 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;
>

Do you mean to say that actually we are just prefetching or reading
the pages in count_nondeletable_pages() but the message doesn't have
any such indication?  If not that, what problem do you see with the
message?   What is your suggestion?

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