Re: error context for vacuum to include block number

On Mon, 17 Feb 2020 at 15:14, Justin Pryzby <pryzby(at)telsasoft(dot)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