Re: error context for vacuum to include block number

On Sat, 15 Feb 2020 at 00:34, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Fri, Feb 14, 2020 at 12:30:25PM +0900, Masahiko Sawada wrote:
> > * 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"?
>
> > * 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.
>
> Fixed these
>
> > * rel->rd_index->indexrelid should be rel->rd_index->indrelid.
>
> Ack.  I think that's been wrong since I first wrote it two weeks ago :(
> The error is probably more obvious due to the switch statement you proposed.
>
> Thanks for continued reviews.

Thank you for updating the patch!

1.
+   /* Setup error traceback support for ereport() */
+   init_vacuum_error_callback(&errcallback, &errcbarg, onerel,
PROGRESS_VACUUM_PHASE_SCAN_HEAP);

+   /*
+    * Setup error traceback support for ereport()
+    */
+   init_vacuum_error_callback(&errcallback, &errcbarg, onerel,
PROGRESS_VACUUM_PHASE_VACUUM_HEAP);

+   /* Setup error traceback support for ereport() */
+   init_vacuum_error_callback(&errcallback, &errcbarg, indrel,
PROGRESS_VACUUM_PHASE_VACUUM_INDEX);

+   /* Setup error traceback support for ereport() */
+   init_vacuum_error_callback(&errcallback, &errcbarg, indrel,
PROGRESS_VACUUM_PHASE_INDEX_CLEANUP);

+/* Initialize vacuum error callback */
+static void
+init_vacuum_error_callback(ErrorContextCallback *errcallback,
vacuum_error_callback_arg *errcbarg, Relation rel, int phase)

The above lines need a new line.

2.
static void
lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
{
    int         tupindex;
    int         npages;
    PGRUsage    ru0;
    Buffer      vmbuffer = InvalidBuffer;
    ErrorContextCallback errcallback;
    vacuum_error_callback_arg errcbarg;

    /* Report that we are now vacuuming the heap */
    pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
                                 PROGRESS_VACUUM_PHASE_VACUUM_HEAP);

    /*
     * Setup error traceback support for ereport()
     */
    init_vacuum_error_callback(&errcallback, &errcbarg, onerel,
PROGRESS_VACUUM_PHASE_VACUUM_HEAP);
    error_context_stack = &errcallback;

    pg_rusage_init(&ru0);
    npages = 0;
    :

In lazy_vacuum_heap, we set the error context and then call
pg_rusage_init whereas lazy_vacuum_index and lazy_cleanup_index does
the opposite. And lazy_scan_heap also call pg_rusage first. I think
lazy_vacuum_heap should follow them for consistency. That is, we can
set error context after pages = 0.

3.
We have 2 other phases: PROGRESS_VACUUM_PHASE_TRUNCATE and
PROGRESS_VACUUM_PHASE_FINAL_CLEANUP. I think it's better to set the
error context in lazy_truncate_heap as well. What do you think?

I'm not sure it's worth to set the error context for FINAL_CLENAUP but
we should add the case of FINAL_CLENAUP to vacuum_error_callback as
no-op or explain it as a comment even if we don't.

Regards,

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