Re: error context for vacuum to include block number

On Thu, Feb 27, 2020 at 09:09:42PM -0300, Alvaro Herrera wrote:
> > + case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
> > + if (BlockNumberIsValid(cbarg->blkno))
> > + errcontext("while vacuuming block %u of relation \"%s.%s\"",
> > + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> > + break;
>
> I think you should still call errcontext() when blkno is invalid.

In my experience while testing, the conditional avoids lots of CONTEXT noise
from interrupted autovacuum, at least. I couldn't easily reproduce it with the
current patch, though, maybe due to less pushing and popping.

> Maybe it would make sense to make the LVRelStats struct members be char
> arrays rather than pointers. Then you memcpy() or strlcpy() them
> instead of palloc/free.

I had done that in the v15 patch, to allow passing it to parallel workers.
But I don't think it's really needed.

On Tue, Mar 03, 2020 at 10:05:42PM +0900, Masahiko Sawada wrote:
> 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.

Done in the attached. But I think non-error reporting of additional progress
phases is out of scope for this patch.

> On Fri, 21 Feb 2020 at 02:02, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> > 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.

In the previous patch, I added this to vacuum_one_index. But I noticed that
sometimes reported multiple CONTEXT lines (while vacuuming..while scanning),
which isn't intended. I was hacked around that by setting ->previous=NULL, but
your way in parallel main() seems better.

--
Justin