On Mon, Mar 16, 2020 at 7:47 PM Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > > On 2020-Mar-16, Amit Kapila wrote: > > > 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. > > Heh, he had that and I took it away -- it looked unnatural. I thought > changing error_context_stack inside such a function, then resetting it > back to "previous" outside the function, was too leaky an abstraction. >
We could have something like setup_parser_errposition_callback and cancel_parser_errposition_callback which might look a bit better. I thought to avoid having similar code at different places and it might look a bit cleaner especially because we are adding code to an already large function like lazy_scan_heap(), but if you don't like the idea, then we can leave it as it is. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
