Re: error context for vacuum to include block number

On Thu, Feb 20, 2020 at 02:02:36PM -0300, Alvaro Herrera wrote:
> 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?

Hm, looks like you're right, but that's not what I intended (and I didn't hit
that in my test).

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

Originally, the patch only supported "scanning heap", and set the callback
strictly, to avoid having callback installed when calling other functions (like
vacuuming heap/indexes).

Then incrementally added callbacks in increasing number of places.  We only
need one errcontext.  And possibly you're right that the callback could always
be in place (?).  But what about things like vacuuming FSM ?  I think we'd need
another "phase" for that (or else invent a PHASE_IGNORE to do nothing).  Would
VACUUM_FSM be added to progress reporting, too?  We're also talking about new
phase for TRUNCATE_PREFETCH and TRUNCATE_WAIT.

Regarding the cbarg, at one point I took a suggestion from Andres to use the
LVRelStats struct.  I got rid of that since I didn't like sharing "blkno"
between heap scanning and heap vacuuming, and needs to be reset when switching
back to scanning heap.  I experimented now going back to that now.  The only
utility is in having an single allocation of relname/space.

-- 
Justin