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
