Re: error context for vacuum to include block number

On Sun, 2 Feb 2020 at 15:03, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> Thanks for reviewing again
>
> On Sun, Feb 02, 2020 at 10:45:12AM +0900, Masahiko Sawada wrote:
> > Thank you for updating the patch. Here is some review comments:
> >
> > 1.
> > +typedef struct
> > +{
> > + char *relnamespace;
> > + char *relname;
> > + char *indname; /* If vacuuming index */
> >
> > I think "Non-null if vacuuming index" is better.
>
> Actually it's undefined garbage (not NULL) if not vacuuming index.

So how about something like "set index name only during vacuuming
index". My point is that the current comment seems to be unclear to me
what describing.

>
> > And tablename is better than relname for accuracy?
>
> The existing code uses relname, so I left that, since it's strange to
> start using tablename and then write things like:
>
> | errcbarg.tblname = relname;
> ...
> | errcontext(_("while scanning block %u of relation \"%s.%s\""),
> | cbarg->blkno, cbarg->relnamespace, cbarg->tblname);
>
> Also, mat views can be vacuumed.

ok, agreed.

>
> > 2.
> > + BlockNumber blkno;
> > + int stage; /* 0: scan heap; 1: vacuum heap; 2: vacuum index */
> > +} vacuum_error_callback_arg;
> >
> > Why do we not support index cleanup phase?
>
> The patch started out just handling scan_heap. The added vacuum_heap. Then
> added vacuum_index. Now, I've added index cleanup.
>
> > 4.
> > + /*
> > + * Setup error traceback support for ereport()
> > + * ->relnamespace and ->relname are already set
> > + */
> > + errcbarg.blkno = InvalidBlockNumber; /* Not known yet */
> > + errcbarg.stage = 1;
> >
> > relnamespace and relname of errcbarg is not set as it is initialized
> > in this function.
>
> Thanks. That's an oversight from switching back to local vars instead of
> LVRelStats while updating the patch while out of town..
>
> I don't know how to consistently test the vacuum_heap case, but rechecked it just now.
>
> postgres=# SET client_min_messages=debug; SET statement_timeout=0; UPDATE t SET a=1+a; SET statement_timeout=150; VACUUM(VERBOSE, PARALLEL 1) t;
> ...
> 2020-02-01 23:11:06.482 CST [26609] ERROR: canceling statement due to statement timeout
> 2020-02-01 23:11:06.482 CST [26609] CONTEXT: while vacuuming block 33 of relation "public.t"
>
> > 5.
> > @@ -177,6 +177,7 @@ typedef struct LVShared
> > * the lazy vacuum.
> > */
> > Oid relid;
> > + char relname[NAMEDATALEN]; /* tablename, used for error callback */
> >
> > How about getting relation
> > name from index relation? That is, in lazy_vacuum_index we can get
> > table oid from the index relation by IndexGetRelation() and therefore
> > we can get the table name which is in palloc'd memory. That way we
> > don't need to add relname to any existing struct such as LVRelStats
> > and LVShared.
>
> See attached
>
> Also, I think we shouldn't show a block number if it's "Invalid", to avoid
> saying "while vacuuming block 4294967295 of relation ..."
>
> For now, I made it not show any errcontext at all in that case.

Thank you for updating the patch!

Here is the comment for v16 patch:

1.
+ ErrorContextCallback errcallback = { error_context_stack,
vacuum_error_callback, &errcbarg, };

I think it's better to initialize individual fields because we might
need to fix it as well when fields of ErrorContextCallback are
changed.

2.
+ /* Replace error context while continuing heap scan */
+ error_context_stack = &errcallback;

/*
* Forget the now-vacuumed tuples, and press on, but be careful
* not to reset latestRemovedXid since we want that value to be
* valid.
*/
dead_tuples->num_tuples = 0;

/*
* Vacuum the Free Space Map to make newly-freed space visible on
* upper-level FSM pages. Note we have not yet processed blkno.
*/
FreeSpaceMapVacuumRange(onerel, next_fsm_block_to_vacuum, blkno);
next_fsm_block_to_vacuum = blkno;

/* Report that we are once again scanning the heap */
pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
PROGRESS_VACUUM_PHASE_SCAN_HEAP);
}

I think we can set the error context for heap scan again after
freespace map vacuum finishing, maybe after reporting the new phase.
Otherwise the user will get confused if an error occurs during
freespace map vacuum. And I think the comment is unclear, how about
"Set the error context fro heap scan again"?

3.
+ if (cbarg->blkno!=InvalidBlockNumber)
+ errcontext(_("while scanning block %u of relation \"%s.%s\""),
+ cbarg->blkno, cbarg->relnamespace, cbarg->relname);

We can use BlockNumberIsValid macro instead.

Regards,

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