Re: error context for vacuum to include block number

On Thu, Mar 26, 2020 at 10:04:57AM -0500, Justin Pryzby wrote:
> Does that address your comment ?

I hope so.

> > I'm not sure why "free_oldindname" is necessary. Since we initialize
> > vacrelstats->indname with NULL and revert the callback arguments at
> > the end of functions that needs update them, vacrelstats->indname is
> > NULL at the beginning of lazy_vacuum_index() and lazy_cleanup_index().
> > And we make a copy of index name in update_vacuum_error_cbarg(). So I
> > think we can pfree the old index name if errcbarg->indname is not NULL.
> 
> We want to avoid doing this:
>  olderrcbarg = *vacrelstats // saves a pointer
>  update_vacuum_error_cbarg(... NULL); // frees the pointer and sets indname to NULL
>  update_vacuum_error_cbarg(... olderrcbarg.oldindnam) // puts back the pointer, which has been freed
>  // hit an error, and the callback accesses the pfreed pointer
> 
> I think that's only an issue for lazy_vacuum_index().
> 
> And I think you're right: we only save state when the calling function has a
> indname=NULL, so we never "put back" a non-NULL indname.  We go from having a
> indname=NULL at lazy_scan_heap to not not-NULL at lazy_vacuum_index, and never
> the other way around.  So once we've "reverted back", 1) the pointer is null;
> and, 2) the callback function doesn't access it for the previous/reverted phase
> anyway.

I removed the free_oldindname argument.

> Hm, I was just wondering what happens if an error happens *during*
> update_vacuum_error_cbarg().  It seems like if we set
> errcbarg->phase=VACUUM_INDEX before setting errcbarg->indname=indname, then an
> error would cause a crash.  And if we pfree and set indname before phase, it'd
> be a problem when going from an index phase to non-index phase.  So maybe we
> have to set errcbarg->phase=VACUUM_ERRCB_PHASE_UNKNOWN while in the function,
> and errcbarg->phase=phase last.

And addressed that.

Also, I realized that lazy_cleanup_index has an early "return", so the "Revert
back" was ineffective.  We talked about how that wasn't needed, since we never
go back to a previous phase.  Amit wanted to keep it there for consistency, but
I'd prefer to put any extra effort into calling out the special treatment
needed/given to lazy_vacuum_heap/index, rather than making everything
"consistent".

Amit: I also moved the TRUNCATE_HEAP bit back to truncate_heap(), since 1) it's
odd if we don't have anything in truncate_heap() about error reporting except
for "vacrelstats->blkno = blkno"; and, 2) it's nice to set the err callback arg
right after pgstat_progress, and outside of any loop.  In previous versions, it
was within the loop, because it closely wrapped RelationTruncate() and
count_nondeletable_pages() - a previous version used separate phases.

-- 
Justin