On Mon, Mar 16, 2020 at 7:47 PM Alvaro Herrera <alvherre@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
