From: Justin Pryzby <pryzby@telsasoft.com>
To: Mahendra Singh Thalor <mahi6run@gmail.com>
Cc: pgsql-hackers@postgresql.org, Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Subject: Re: display offset along with block number in vacuum errors
Date: Fri, 31 Jul 2020 16:55:14 -0500
Message-ID: <20200731215514.GC20393@telsasoft.com> (raw)
In-Reply-To: <CAKYtNApLJjAaRw0UEBBY6G1o0LRZKS7rA5n46BFh+NfwSOycdg@mail.gmail.com>
References: <CAKYtNApK488TDF4bMbw+1QH8HJf9cxdNDXquhU50TK5iv_FtCQ@mail.gmail.com>
<20200727111352.GN4286@telsasoft.com>
<CAKYtNApLJjAaRw0UEBBY6G1o0LRZKS7rA5n46BFh+NfwSOycdg@mail.gmail.com>
Bcc:
Subject: Re: display offset along with block number in vacuum errors
Reply-To:
In-Reply-To: <CAKYtNApLJjAaRw0UEBBY6G1o0LRZKS7rA5n46BFh+NfwSOycdg@mail.gmail.com>
On Wed, Jul 29, 2020 at 12:35:17AM +0530, Mahendra Singh Thalor wrote:
> > Here:
> >
> > @@ -1924,14 +1932,22 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
> > BlockNumber tblk;
> > OffsetNumber toff;
> > ItemId itemid;
> > + LVSavedErrInfo loc_saved_err_info;
> >
> > tblk = ItemPointerGetBlockNumber(&dead_tuples->itemptrs[tupindex]);
> > if (tblk != blkno)
> > break; /* past end of tuples for this block */
> > toff = ItemPointerGetOffsetNumber(&dead_tuples->itemptrs[tupindex]);
> > +
> > + /* Update error traceback information */
> > + update_vacuum_error_info(vacrelstats, &loc_saved_err_info, VACUUM_ERRCB_PHASE_VACUUM_HEAP,
> > + blkno, toff);
> > itemid = PageGetItemId(page, toff);
> > ItemIdSetUnused(itemid);
> > unused[uncnt++] = toff;
> > +
> > + /* Revert to the previous phase information for error traceback */
> > + restore_vacuum_error_info(vacrelstats, &loc_saved_err_info);
> > }
> >
> > I'm not sure why you use restore_vacuum_error_info() at all. It's already
> > called at the end of lazy_vacuum_page() (and others) to allow functions to
> > clean up after their own state changes, rather than requiring callers to do it.
> > I don't think you should use it in a loop, nor introduce another
> > LVSavedErrInfo.
> >
> > Since phase and blkno are already set, I think you should just set
> > vacrelstats->offnum = toff, rather than calling update_vacuum_error_info().
> > Similar to whats done in lazy_vacuum_heap():
> >
> > tblk = ItemPointerGetBlockNumber(&vacrelstats->dead_tuples->itemptrs[tupindex]);
> > vacrelstats->blkno = tblk;
>
> Fixed.
I rearead this thread and I think the earlier suggestion from Masahiko was
right. The loop around dead_tuples only does ItemIdSetUnused() which updates
the page, which has already been read from disk. On my suggestion, your v2
patch sets offnum directly, but now I think it's not useful to set at all,
since the whole page is manipulated by PageRepairFragmentation() and
log_heap_clean(). An error there would misleadingly say "..at offset number
MM", but would always show the page's last offset, and not the offset where an
error occured.
I added this at:
https://commitfest.postgresql.org/29/2662/
If anyone is considering this patch for v13, I guess it should be completed by
next week.
--
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, mahi6run@gmail.com, masahiko.sawada@2ndquadrant.com
Subject: Re: display offset along with block number in vacuum errors
In-Reply-To: <20200731215514.GC20393@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