Re: error context for vacuum to include block number

On Wed, 22 Jan 2020 at 10:17, Justin Pryzby <pryzby(at)telsasoft(dot)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