Re: error context for vacuum to include block number

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.

> 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.

> 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.