On Mon, 17 Feb 2020 at 15:14, Justin Pryzby <pry...@telsasoft.com> wrote: > > On Mon, Feb 17, 2020 at 02:18:11PM +0900, Masahiko Sawada wrote: > > Oops it seems to me that it's better to set error context after > > tupindex = 0. Sorry for my bad. > > I take your point but did it differently - see what you think > > > And the above comment can be written in a single line as other same > > comments. > > Thanks :) > > > Hmm I don't think it's a good idea to have count_nondeletable_pages > > set the error context of PHASE_TRUNCATE. > > I think if we don't do it there then we shouldn't bother handling > PHASE_TRUNCATE at all, since that's what's likely to hit filesystem or other > lowlevel errors, before lazy_truncate_heap() hits them. > > > Because the patch sets the > > error context during RelationTruncate that actually truncates the heap > > but count_nondeletable_pages doesn't truncate it. > > I would say that ReadBuffer called by the prefetch in > count_nondeletable_pages() is called during the course of truncation, the same > as ReadBuffer called during the course of vacuuming can be attributed to > vacuuming.
Why do we want to include only count_nondeletable_pages in spite of that there are also several places where we may wait: waiting for lock, get the number of blocks etc. User may cancel vacuum during them but user will not be able to know that vacuum is in truncation phase. If we want to set the error callback during operation that actually doesn't truncate heap like count_nondeletable_pages we should set it for whole lazy_truncate_heap. Otherwise I think we should set it for only RelationTruncate. > > > I think setting the error context only during RelationTruncate would be a > > good start. We can hear other opinions from other hackers. Some hackers may > > want to set the error context for whole lazy_truncate_heap. > > I avoided doing that since it has several "return" statements, each of which > would need to "Pop the error context stack", which is at risk of being > forgotten and left unpopped by anyone who adds or changes flow control. I imagined that we can add some goto and pop the error callback there. But since it might make the code bad I suggested to set the error callback for only RelationTruncate as the first step Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
