Re: error context for vacuum to include block number

On Fri, 21 Feb 2020 at 02:02, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> This is, by far, the most complex error context callback we've tried to
> write ... Easy stuff first:
>
> In the error context function itself, you don't need the _() around the
> strings: errcontext() is marked as a gettext trigger and it does the
> translation itself, so the manually added _() is just cruft.
>
> When reporting index names, make sure to attach the namespace to the
> table, not to the index.  Example:
>
>         case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
> -           errcontext(_("while cleaning up index \"%s.%s\" of relation \"%s\""),
> -                      cbarg->relnamespace, cbarg->indname, cbarg->relname);
> +           errcontext("while cleaning up index \"%s\" of relation \"%s.%s\"",
> +                      cbarg->indname, cbarg->relnamespace, cbarg->relname);
>
> I think it would be worthwhile to have the "truncate wait" phase as a
> separate thing from the truncate itself, since it requires acquiring a
> possibly taken lock.  This suggests that using the progress enum is not
> a 100% solution ... or maybe it suggests that the progress enum too
> needs to report the truncate-wait phase separately.  (I like the latter
> myself, actually.)
>
> On 2020-Feb-19, Justin Pryzby wrote:
>
> > Also, I was thinking that lazy_scan_heap doesn't needs to do this:
> >
> > +           /* Pop the error context stack while calling vacuum */
> > +           error_context_stack = errcallback.previous;
> > ...
> > +           /* Set the error context while continuing heap scan */
> > +           error_context_stack = &errcallback;
> >
> > It seems to me that's not actually necessary, since lazy_vacuum_heap will just
> > *push* a context handler onto the stack, and then pop it back off.
>
> So if you don't pop before pushing, you'll end up with two context
> lines, right?
>
> I find that arrangement a bit confusing.  I think it would make sense to
> initialize the context callback just *once* for a vacuum run, and from
> that point onwards, just update the errcbarg struct to match what
> you're currently doing -- not continually pop/push error callback stack
> entries.  See below ...

I was concerned about fsm vacuum; vacuum error context might show heap
scan while actually doing fsm vacuum. But perhaps we can update
callback args for that. That would be helpful for user to distinguish
that the problem seems to be either in heap vacuum or in fsm vacuum.

>
> (This means you need to pass the "cbarg" as new argument to some of the
> called functions, so that they can update it.)
>
> Another point is that this patch seems to be leaking memory each time
> you set relation/index/namespace name, since you never free those and
> they are changed over and over.
>
> In init_vacuum_error_callback() you don't need the "switch(phase)" bit;
> instead, test rel->rd_rel->relkind, and if it's RELKIND_INDEX then you
> put the relname as indexname, otherwise set it to NULL (after freeing
> the previous value, if there's one).  Note that with this, you only need
> to set the relation name (table name) in the first call!  IOW you should
> split init_vacuum_error_callback() in two functions: one "init" to call
> at start of vacuum, where you set relnamespace and relname; the other
> function is update_vacuum_error_callback() (or you find a better name
> for that) and it sets the phase, and optionally the block number and
> index name (these last two get reset to InvalidBlkNum/ NULL if not
> passed by caller).  I'm not really sure what this means for the parallel
> index vacuuming stuff; probably you'll need a special case for that: the
> parallel children will need to "init" on their own, right?

Right. In that case, I think parallel vacuum worker needs to init the
callback args at parallel_vacuum_main(). Other functions that parallel
vacuum worker could call are also called by the leader process.

Regards,


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