Re: error context for vacuum to include block number

On Mon, Jan 27, 2020 at 03:59:58PM +0900, Masahiko Sawada wrote:
> On Mon, 27 Jan 2020 at 14:38, Justin Pryzby <pryzby@telsasoft.com> wrote:
> > On Sun, Jan 26, 2020 at 12:29:38PM -0800, Andres Freund wrote:
> > > > CONTEXT:  while vacuuming relation "public.t_a_idx"
> > >
> > > It'd be a bit nicer if it said index "public.t_a_idx" for relation "public.t".
> >
> > I think that tips the scale in favour of making vacrelstats a global.
> > I added that as a 1st patch, and squished the callback patches into one.
> 
> Hmm I don't think it's a good idea to make vacrelstats global. If we
> want to display the relation name and its index name in error context
> we might want to define a new struct dedicated for error context
> reporting. That is it has blkno, stage and relation name and schema
> name for both table and index and then we set these variables of
> callback argument before performing a vacuum phase. We don't change
> LVRelStats at all.

On Mon, Jan 27, 2020 at 12:14:38AM -0600, Justin Pryzby wrote:
> It occured to me that there's an issue with sharing vacrelstats between
> scan/vacuum, since blkno and stage are set by the heap/index vacuum routines,
> but not reset on their return to heap scan.  Not sure if we should reset them,
> or go back to using a separate struct, like it was here:
> https://www.postgresql.org/message-id/20200120054159.GT26045%40telsasoft.com

I went back to this, original, way of doing it.
The parallel vacuum patch made it harder to pass the table around :(
And has to be separately tested:

| SET statement_timeout=0; DROP TABLE t; CREATE TABLE t AS SELECT generate_series(1,99999)a; CREATE INDEX ON t(a);
CREATEINDEX ON t(a); UPDATE t SET a=1+a; SET statement_timeout=99;VACUUM(VERBOSE, PARALLEL 2) t;
 

I had to allocate space for the table name within the LVShared struct, not just
a pointer, otherwise it would variously crash or fail to output the index name.
I think pointers can't be passed to parallel process except using some
heavyweight thing like shm_toc_...

I guess the callback could also take the index relid instead of name, and use
something like IndexGetRelation().

> Although the patch replaces get_namespace_name and
> RelationGetRelationName but we use namespace name of relation at only
> two places and almost ereport/elog messages use only relation name
> gotten by RelationGetRelationName which is a macro to access the
> relation name in Relation struct. So I think adding relname to
> LVRelStats would not be a big benefit. Similarly, adding table
> namespace to LVRelStats would be good to avoid calling
> get_namespace_name whereas I'm not sure it's worth to have it because
> it's expected not to be really many times.

Right, I only tried that to save a few LOC and maybe make shorter lines.
It's not important so I'll drop that patch.