Re: error context for vacuum to include block number

On Sat, 8 Feb 2020 at 10:01, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Tue, Feb 04, 2020 at 01:58:20PM +0900, Masahiko Sawada wrote:
> > Here is the comment for v16 patch:
> >
> > 2.
> > I think we can set the error context for heap scan again after
> > freespace map vacuum finishing, maybe after reporting the new phase.
> > Otherwise the user will get confused if an error occurs during
> > freespace map vacuum. And I think the comment is unclear, how about
> > "Set the error context fro heap scan again"?
>
> Good point
>
> > 3.
> > +           if (cbarg->blkno!=InvalidBlockNumber)
> > +               errcontext(_("while scanning block %u of relation \"%s.%s\""),
> > +                       cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> >
> > We can use BlockNumberIsValid macro instead.
>
> Thanks.  See attached, now squished together patches.
>
> I added functions to initialize the callbacks, so error handling is out of the
> way and minimally distract from the rest of vacuum.

Thank you for updating the patch! Here is the review comments:

1.
+static void vacuum_error_callback(void *arg);
+static void init_error_context_heap(ErrorContextCallback
*errcallback, vacuum_error_callback_arg *errcbarg, Relation onerel,
int phase);
+static void init_error_context_index(ErrorContextCallback
*errcallback, vacuum_error_callback_arg *errcbarg, Relation indrel,
int phase);

You need to add a newline to follow the limit line lengths so that the
code is readable in an 80-column window. Or please run pgindent.

2.
+/* Initialize error context for heap operations */
+static void
+init_error_context_heap(ErrorContextCallback *errcallback,
vacuum_error_callback_arg *errcbarg, Relation onerel, int phase)
+{
+   errcbarg->relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+   errcbarg->relname = RelationGetRelationName(onerel);
+   errcbarg->indname = NULL; /* Not used for heap */
+   errcbarg->blkno = InvalidBlockNumber; /* Not known yet */
+   errcbarg->phase = phase;
+
+   errcallback->callback = vacuum_error_callback;
+   errcallback->arg = errcbarg;
+   errcallback->previous = error_context_stack;
+   error_context_stack = errcallback;
+}

I think that making initialization process of errcontext argument a
function is good. But maybe we can merge these two functions into one.
init_error_context_heap and init_error_context_index actually don't
only initialize the callback arguments but also push the vacuum
errcallback, in spite of the function name having 'init'. Also I think
it might be better to only initialize the callback arguments in this
function and to set errcallback by caller, rather than to wrap pushing
errcallback by a function. How about the following function
initializing the vacuum callback arguments?

static void
init_vacuum_error_callback_arg(vacuum_error_callback_arg *errcbarg,
Relation rel, int phase)
{
   errcbarg->relnamespace = get_namespace_name(RelationGetNamespace(onerel));
   errcbarg->blkno = InvalidBlockNumber;
   errcbarg->phase = phase;

   switch (phase) {
       case PROGRESS_VACUUM_PHASE_SCAN_HEAP:
       case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
           errcbarg->relname = RelationGetRelationName(rel);
           errcbarg->indname = NULL;
           break;

       case PROGRESS_VACUUM_PHASE_VACUUM_INDEX:
       case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
           /* rel is an index relation in index vacuum case */
           errcbarg->relname = get_rel_name(indrel->rd_index->indexrelid);
           errcbarg->indname = RelationGetRelationName(rel);
           break;

   }
}

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services