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.