On Thu, Mar 5, 2020 at 3:22 AM Justin Pryzby <pry...@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
