Re: error context for vacuum to include block number

Thanks for reviewing

On Wed, Jan 22, 2020 at 05:37:06PM +0900, Masahiko Sawada wrote:
> I'm not sure it's worth to have patches separately but I could apply

The later patches expanded on the initial scope, and to my understanding the
1st callback is desirable but the others are maybe still at question.

> 1. * The comment should be updated as we use both relname and
> relnamespace for ereporting.
> 
> * We can leave both blkno and stage that are used only for error
> context reporting put both relname and relnamespace to top of
> LVRelStats.

Updated in the 0005 - still not sure if that patch will be desirable, though.
Also, I realized that it's we cannot use vacrelstats->blkno instead of local
blkno variable, since vacrelstats->blkno is used simultaneously by scan heap
and vacuum heap).

> * The 'stage' is missing to support index cleanup.

But the callback isn't used during index cleanup ?

> * It seems to me strange that only initialization of latestRemovedXid
> is done after error callback initialization.

Yes, that was silly - I thought it was just an artifact of diff's inability to
express rearranged code any better.

> * Maybe we can initialize relname and relnamespace in heap_vacuum_rel
> rather than in lazy_scan_heap. BTW variables of vacrelstats are
> initialized different places: some of them in heap_vacuum_rel and
> others in lazy_scan_heap. I think we can gather those that can be
> initialized at that time to heap_vacuum_rel.

I think that's already true ?  But topic for a separate patch, if not.

> 3. Maybe we can do like:

done

> 4. These comments can be merged like:

done

> 5. Why do we need to initialize blkno in spite of not using?

removed

> 6.
> +               cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> * 'vacrelstats' would be a better name than 'cbarg'.

Really?  I'd prefer to avoid repeating long variable three times:

            vacrelstats->blkno, vacrelstats->relnamespace, vacrelstats->relname);

> * In index vacuum, how about "while vacuuming index \"%s.%s\""?

Yes.  I'm still unclear if this is useful without a block number, or why it
wouldn't be better to write DEBUG1 log with index name before vacuuming each.

Justin