Re: error context for vacuum to include block number

On Wed, 25 Mar 2020 at 12:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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?

I meant that with the patch, suppose that the table has 100 blocks and
we're truncating it to 50 blocks in RelationTruncate(), the error
context message will be "while truncating relation "aaa.bbb" to 100
blocks", which is not correct. I think it should be "while truncating
relation "aaa.bbb" to 50 blocks". We can know the relation can be
truncated to 50 blocks by the result of count_nondeletable_pages(). So
if we update the arguments before it we will use the number of blocks
of relation before truncation.

My suggestion is either that we change the error message to, for
example, "while truncating relation "aaa.bbb" having 100 blocks", or
that we change the patch so that we can use "50 blocks" in the error
context message.

Regards,

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