From: Justin Pryzby <pryzby@telsasoft.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Mahendra Singh Thalor <mahi6run@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: display offset along with block number in vacuum errors
Date: Thu, 6 Aug 2020 09:21:16 -0500
Message-ID: <20200806142116.GL28072@telsasoft.com> (raw)
In-Reply-To: <CAA4eK1+G57j3HQg9zBZXPWWpsG6vt3COeG-fe5dFQ2Ao6J0bCg@mail.gmail.com>
References: <CAKYtNApK488TDF4bMbw+1QH8HJf9cxdNDXquhU50TK5iv_FtCQ@mail.gmail.com>
<20200727111352.GN4286@telsasoft.com>
<CAKYtNApLJjAaRw0UEBBY6G1o0LRZKS7rA5n46BFh+NfwSOycdg@mail.gmail.com>
<20200728193919.GK20393@telsasoft.com>
<CAA4eK1+G57j3HQg9zBZXPWWpsG6vt3COeG-fe5dFQ2Ao6J0bCg@mail.gmail.com>
On Thu, Aug 06, 2020 at 07:39:21PM +0530, Amit Kapila wrote:
> On Wed, Jul 29, 2020 at 1:09 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > On Wed, Jul 29, 2020 at 12:35:17AM +0530, Mahendra Singh Thalor wrote:
> > > Apart from these, I fixed comments given by Sawada and Michael in the
> > > latest patch. Attaching v2 patch for review.
> >
> > Thanks.
> >
> > lazy_check_needs_freeze iterates over blocks and this patch changes it to
> > update vacrelstats. I think it should do what
> > lazy_{vacuum/cleanup}_heap/page/index do and call update_vacuum_error_info() at
> > its beginning (even though only the offset is changed), and then
> > restore_vacuum_error_info() at its end (to "revert back" to the item number it
> > started with).
> >
>
> I see that Mahendra has changed patch as per this suggestion but I am
> not convinced that it is a good idea to sprinkle
> update_vacuum_error_info()/restore_vacuum_error_info() at places more
> than required. I see that it might look a bit clean from the
> perspective that if tomorrow we use the function
> lazy_check_needs_freeze() for a different purpose then we don't need
> to worry about the wrong phase information. If we are worried about
> that then we should have an assert in that function to ensure that the
> current phase is VACUUM_ERRCB_PHASE_SCAN_HEAP.
The motivation was to restore the offnum, which is set to Invalid at the start
of lazy_scan_heap(), and then set valid within lazy_check_needs_freeze, but
should be restored or re-set to Invalid when returns to lazy_scan_heap(). If
you think it's important, we could just set vacrelstats->offnum = Invalid
before returning, but that's what the restore function was built for. We do
direct assignment in 2 places to avoid a function call within a loop.
lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
Relation *Irel, int nindexes, bool aggressive)
...
for (blkno = 0; blkno < nblocks; blkno++)
{
...
update_vacuum_error_info(vacrelstats, NULL, VACUUM_ERRCB_PHASE_SCAN_HEAP,
blkno, InvalidOffsetNumber);
if (!ConditionalLockBufferForCleanup(buf))
{
...
if (!lazy_check_needs_freeze(buf, &hastup, vacrelstats))
{
...
for (offnum = FirstOffsetNumber;
offnum <= maxoff;
offnum = OffsetNumberNext(offnum))
--
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, mahi6run@gmail.com
Subject: Re: display offset along with block number in vacuum errors
In-Reply-To: <20200806142116.GL28072@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