Re: error context for vacuum to include block number

On Tue, 28 Jan 2020 at 07:50, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> On Mon, Jan 27, 2020 at 03:59:58PM +0900, Masahiko Sawada wrote:
> > On Mon, 27 Jan 2020 at 14:38, Justin Pryzby <pryzby(at)telsasoft(dot)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); CREATE INDEX 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.

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. And tablename is
better than relname for accuracy?

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?

3.
/* 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);

I think it's an unnecessary removal.

4.
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()
+ * ->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.

5.
@@ -177,6 +177,7 @@ typedef struct LVShared
* the lazy vacuum.
*/
Oid relid;
+ char relname[NAMEDATALEN]; /* tablename, used for error callback */

Hmm I think it's not a good idea to have LVShared have relname because
the parallel vacuum worker being able to know the table name by oid
and it consumes DSM memory. To pass the relation name down to
lazy_vacuum_index I thought to add new argument relname to some
functions but in parallel vacuum case there are multiple functions
until we reach lazy_vacuum_index. So I think it doesn't make sense to
add a new argument to all those functions. 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.

Regards,

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