On Fri, Aug 14, 2020 at 4:06 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Aug 10, 2020 at 10:24 AM Masahiko Sawada > <masahiko.saw...@2ndquadrant.com> wrote: > > > > It's true that heap_page_is_all_visible() is called from only > > lazy_vacuum_page() but I'm concerned it would lead misleading since > > it's not actually removing tuples but just checking after vacuum. I > > guess that the errcontext should show what the process is actually > > doing and therefore help the investigation, so I thought VACUUM_HEAP > > might not be appropriate for this case. But I see also your point. > > Other vacuum error context phases match with vacuum progress > > information phrases. So in heap_page_is_all_visible (), I agree it's > > better to update the offset number and keep the phase VACUUM_HEAP > > rather than do nothing. > > > > Okay, I have changed accordingly and this means that the offset will > be displayed for the vacuum phase as well. Apart from this, I have > fixed all the comments raised by me in the attached patch. One thing > we need to think is do we want to set offset during heap_page_prune > when called from lazy_scan_heap? I think the code path for > heap_prune_chain is quite deep, so an error can occur in that path. > What do you think? >
The reason why I have not included heap_page_prune related change in the patch is that I don't want to sprinkle this in every possible function (code path) called via vacuum especially if the probability of an error in that code path is low. But, I am fine if you and or others think that it is a good idea to update offset in heap_page_prune as well. -- With Regards, Amit Kapila.