Re: error context for vacuum to include block number - Mailing list pgsql-hackers

On Wed, 22 Jan 2020 at 10:17, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Tue, Jan 21, 2020 at 05:54:59PM -0300, Alvaro Herrera wrote:
> > > On Tue, Jan 21, 2020 at 03:11:35PM +0900, Masahiko Sawada wrote:
> > > > Some of them conflicts with the current HEAD(62c9b52231). Please rebase them.
> > >
> > > Sorry, it's due to other vacuum patch in this branch.
> > >
> > > New patches won't conflict, except for the 0005, so I renamed it for cfbot.
> > > If it's deemed to be useful, I'll make a separate branch for the others.
> >
> > I think you have to have some other patches applied before these,
> > because in the context lines for some of the hunks there are
> > double-slash comments.
>
> And I knew that, so (re)tested that the extracted patches would apply, but it
> looks like maybe the patch checker is less smart (or more strict) than git, so
> it didn't work after all.

Thank you for updating the patches!

I'm not sure it's worth to have patches separately but I could apply
all patches cleanly. Here is my comments for the code applied all
patches:

1.
+   /* Used by the error callback */
+   char        *relname;
+   char        *relnamespace;
+   BlockNumber blkno;
+   int         stage;  /* 0: scan heap; 1: vacuum heap; 2: vacuum index */
+} LVRelStats;

* The comment should be updated as we use both relname and
relnamespace for ereporting.

* We can leave both blkno and stage that are used only for error
context reporting put both relname and relnamespace to top of
LVRelStats.

* The 'stage' is missing to support index cleanup.

* Maybe we need a comment for 'blkno'.

2.
@@ -748,8 +742,31 @@ lazy_scan_heap(Relation onerel, VacuumParams
*params, LVRelStats *vacrelstats,
    vacrelstats->scanned_pages = 0;
    vacrelstats->tupcount_pages = 0;
    vacrelstats->nonempty_pages = 0;
+
+   /* Setup error traceback support for ereport() */
+   vacrelstats->relnamespace =
get_namespace_name(RelationGetNamespace(onerel));
+   vacrelstats->relname = RelationGetRelationName(onerel);
+   vacrelstats->blkno = InvalidBlockNumber; /* Not known yet */
+   vacrelstats->stage = 0;
+
+   errcallback.callback = vacuum_error_callback;
+   errcallback.arg = (void *) vacrelstats;
+   errcallback.previous = error_context_stack;
+   error_context_stack = &errcallback;
+
    vacrelstats->latestRemovedXid = InvalidTransactionId;

+   if (aggressive)
+       ereport(elevel,
+               (errmsg("aggressively vacuuming \"%s.%s\"",
+                       vacrelstats->relnamespace,
+                       vacrelstats->relname)));
+   else
+       ereport(elevel,
+               (errmsg("vacuuming \"%s.%s\"",
+                       vacrelstats->relnamespace,
+                       vacrelstats->relname)));

* It seems to me strange that only initialization of latestRemovedXid
is done after error callback initialization.

* Maybe we can initialize relname and relnamespace in heap_vacuum_rel
rather than in lazy_scan_heap. BTW variables of vacrelstats are
initialized different places: some of them in heap_vacuum_rel and
others in lazy_scan_heap. I think we can gather those that can be
initialized at that time to heap_vacuum_rel.

3.
            /* Work on all the indexes, then the heap */
+           /* Don't use the errcontext handler outside this function */
+           error_context_stack = errcallback.previous;
            lazy_vacuum_all_indexes(onerel, Irel, indstats,
                                    vacrelstats, lps, nindexes);
-
            /* Remove tuples from heap */
            lazy_vacuum_heap(onerel, vacrelstats);
+           error_context_stack = &errcallback;

Maybe we can do like:

           /* Pop the error context stack */
           error_context_stack = errcallback.previous;

            /* Work on all the indexes, then the heap */
            lazy_vacuum_all_indexes(onerel, Irel, indstats,
                                    vacrelstats, lps, nindexes);

            /* Remove tuples from heap */
            lazy_vacuum_heap(onerel, vacrelstats);

            /* Push again the error context of heap scan */
            error_context_stack = &errcallback;

4.
+   /* Setup error traceback support for ereport() */
+   /* vacrelstats->relnamespace already set */
+   /* vacrelstats->relname already set */

These comments can be merged like:

/*
 * Setup error traceback for ereport(). Both relnamespace and
 * relname are already set.
 */

5.
+   /* Setup error traceback support for ereport() */
+   vacrelstats.relnamespace = get_namespace_name(RelationGetNamespace(indrel));
+   vacrelstats.relname = RelationGetRelationName(indrel);
+   vacrelstats.blkno = InvalidBlockNumber; /* Not used */

Why do we need to initialize blkno in spite of not using?

6.
+/*
+ * Error context callback for errors occurring during vacuum.
+ */
+static void
+vacuum_error_callback(void *arg)
+{
+   LVRelStats *cbarg = arg;
+
+   if (cbarg->stage == 0)
+       errcontext(_("while scanning block %u of relation \"%s.%s\""),
+               cbarg->blkno, cbarg->relnamespace, cbarg->relname);
+   else if (cbarg->stage == 1)
+       errcontext(_("while vacuuming block %u of relation \"%s.%s\""),
+               cbarg->blkno, cbarg->relnamespace, cbarg->relname);
+   else if (cbarg->stage == 2)
+       errcontext(_("while vacuuming relation \"%s.%s\""),
+               cbarg->relnamespace, cbarg->relname);
+}

* 'vacrelstats' would be a better name than 'cbarg'.

* In index vacuum, how about "while vacuuming index \"%s.%s\""?

Regards,

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