Re: error context for vacuum to include block number

On Fri, 14 Feb 2020 at 08:52, Justin Pryzby <pryzby@telsasoft.com> wrote:
>

Thank you for updating the patch.

> On Thu, Feb 13, 2020 at 02:55:53PM +0900, Masahiko Sawada wrote:
> > 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.
>
> For now I :set tw=80
>
> > 2.
> > I think that making initialization process of errcontext argument a
> > function is good. But maybe we can merge these two functions into one.
>
> Thanks, this is better, and I used that.
>
> > 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.
>
> However I think it's important not to repeat this 4 times:
>         errcallback->callback = vacuum_error_callback;
>         errcallback->arg = errcbarg;
>         errcallback->previous = error_context_stack;
>         error_context_stack = errcallback;
>
> So I kept the first 3 of those in the function and copied only assignment to
> the global.  That helps makes the heap scan function clear, which assigns to it
> twice.

Okay. Here is the review comments for v18 patch:

1.
+/* Initialize error context for heap operations */
+static void
+init_error_context(ErrorContextCallback *errcallback,
vacuum_error_callback_arg *errcbarg, Relation rel, int phase)

* I think the function name is too generic. init_vacuum_error_callback
or init_vacuum_errcallback is better.

* The comment of this function is not accurate since this function is
not only for heap vacuum but also index vacuum. How about just
"Initialize vacuum error callback"?

2.
+{
+       switch (phase)
+       {
+               case PROGRESS_VACUUM_PHASE_SCAN_HEAP:
+               case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
+                       errcbarg->relname = RelationGetRelationName(rel);
+                       errcbarg->indname = NULL; /* Not used for heap */
+                       break;
+
+               case PROGRESS_VACUUM_PHASE_VACUUM_INDEX:
+               case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
+                       /* indname is the index being processed,
relname is its relation */
+                       errcbarg->indname = RelationGetRelationName(rel);
+                       errcbarg->relname =
get_rel_name(rel->rd_index->indexrelid);

* I think it's easier to read the code if we set the relname and
indname in the same order.

* The comment I wrote in the previous mail seems better, because in
this function the reader might get confused that 'rel' is a relation
or an index depending on the phase but that comment helps it.

* rel->rd_index->indexrelid should be rel->rd_index->indrelid.

Regards,

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