Re: error context for vacuum to include block number

On Thu, Mar 5, 2020 at 3:22 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Wed, Mar 04, 2020 at 04:21:06PM +0900, Masahiko Sawada wrote:
> > Thank you for updating the patch. But we have two more places where we
> > do fsm vacuum.
>
> Oops, thanks.
>
> I realized that vacuum_page is called not only from lazy_vacuum_heap, but also
> directly from lazy_scan_heap, which failed to update errcbarg.  So I changed to
> update errcbarg in vacuum_page.
>
> What about these other calls ?  I think granularity of individual function
> calls requires a debugger, but is it fine issue if errors here are attributed
> to (say) "scanning heap" ?
>
> GetRecordedFreeSpace
> heap_*_freeze_tuple
> heap_page_prune
> HeapTupleSatisfiesVacuum
> LockBufferForCleanup
> MarkBufferDirty
> Page*AllVisible
> PageGetHeapFreeSpace
> RecordPageWithFreeSpace
> visibilitymap_*
> VM_ALL_FROZEN
>

I think we can keep granularity the same as we have for progress
update functionality which means "scanning heap" is fine.  On similar
lines, it is not clear whether it is a good idea to keep a phase like
VACUUM_ERRCB_PHASE_VACUUM_FSM as it has added additional updates in
multiple places in the code.

Few other comments:
1.
+ /* Init vacrelstats for use as error callback by parallel worker: */
+ vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(onerel));

It looks a bit odd that the comment is ended with semicolon (:), is
there any reason for same?

2.
+ /* Setup error traceback support for ereport() */
+ update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_SCAN_HEAP,
+ InvalidBlockNumber, NULL);
+ errcallback.callback = vacuum_error_callback;
+ errcallback.arg = vacrelstats;
+ errcallback.previous = error_context_stack;
+ error_context_stack = &errcallback;
..
..
+ /* Init vacrelstats for use as error callback by parallel worker: */
+ vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+ vacrelstats.relname = pstrdup(RelationGetRelationName(onerel));
+ vacrelstats.indname = NULL;
+ vacrelstats.phase = VACUUM_ERRCB_PHASE_UNKNOWN; /* Not yet processing */
+
+ /* Setup error traceback support for ereport() */
+ errcallback.callback = vacuum_error_callback;
+ errcallback.arg = &vacrelstats;
+ errcallback.previous = error_context_stack;
+ error_context_stack = &errcallback;
+

I think the code can be bit simplified if we have a function
setup_vacuum_error_ctx which takes necessary parameters and fill the
required vacrelstats params, setup errcallback.  Then we can use
update_vacuum_error_cbarg at required places.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com