Re: error context for vacuum to include block number

On Fri, 14 Feb 2020 at 08:52, Justin Pryzby <pryzby(at)telsasoft(dot)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