From: Justin Pryzby <pryzby@telsasoft.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: error context for vacuum to include block number
Date: Wed, 25 Mar 2020 23:41:15 -0500
Message-ID: <20200326044115.GB28385@telsasoft.com> (raw)
In-Reply-To: <CAA4eK1+=ToXurzjOcPyQ4j6Vz2nG2C-cUdg=yZoUpN+g1k5m6w@mail.gmail.com>
References: <CA+fd4k6KdmyLqMc3GC5QGX9d9WPmDWnUugEfJ7xhyLQRBEmrRA@mail.gmail.com>
<CAA4eK1KRvWEpcvEM9rc_oNi8PFSps-kOvzebxFUE0JXdanjE-g@mail.gmail.com>
<20200325101229.GR21443@telsasoft.com>
<CAA4eK1KwcMWa7Jwewh48xC9o9pExzd=CuRuox94+AHvYr3F7Qg@mail.gmail.com>
<CA+fd4k6-zFG=mvkZzgsLQZ3v+df5Bk_0d8tpSzcRV0u40dM0rw@mail.gmail.com>
<20200325124155.GU21443@telsasoft.com>
<CAA4eK1+=ToXurzjOcPyQ4j6Vz2nG2C-cUdg=yZoUpN+g1k5m6w@mail.gmail.com>
On Thu, Mar 26, 2020 at 09:50:53AM +0530, Amit Kapila wrote:
> > > after count_nondeletable_pages, and then revert it back to
> > > VACUUM_ERRCB_PHASE_SCAN_HEAP phase and the number of blocks of
> > > relation before truncation, after RelationTruncate(). It can be
> > > repeated until no more truncating can be done. Why do we need to
> > > revert back to the scan heap phase? If we can use
> > > vacrelstats->nonempty_pages in the error context message as the
> > > remaining blocks after truncation I think we can update callback
> > > arguments once at the beginning of lazy_truncate_heap() and don't
> > > revert to the previous phase, and pop the error context after exiting.
> >
> > Perhaps. We need to "revert back" for the vacuum phases, which can be called
> > multiple times, but we don't need to do that here.
>
> Yeah, but I think it would be better if are consistent because we have
> no control what the caller of the function intends to do after
> finishing the current phase. I think we can add some comments where
> we set up the context (in heap_vacuum_rel) like below so that the idea
> is more clear.
>
> "The idea is to set up an error context callback to display additional
> information with any error during vacuum. During different phases of
> vacuum (heap scan, heap vacuum, index vacuum, index clean up, heap
> truncate), we update the error context callback to display appropriate
> information.
>
> Note that different phases of vacuum overlap with each other, so once
> a particular phase is over, we need to revert back to the old phase to
> keep the phase information up-to-date."
Seems fine. Rather than saying "different phases" I, would say:
"The index vacuum and heap vacuum phases may be called multiple times in the
middle of the heap scan phase."
But actually I think the concern is not that we unnecessarily "Revert back to
the old phase" but that we do it in a *loop*. Which I agree doesn't make
sense, to go back and forth between "scanning heap" and "truncating". So I
think we should either remove the "revert back", or otherwise put it
after/outside the "while" loop, and change the "return" paths to use "break".
--
Justin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, amit.kapila16@gmail.com, masahiko.sawada@2ndquadrant.com, alvherre@2ndquadrant.com, andres@anarazel.de, michael@paquier.xyz
Subject: Re: error context for vacuum to include block number
In-Reply-To: <20200326044115.GB28385@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox