Re: display offset along with block number in vacuum errors - Mailing list pgsql-hackers
From | Justin Pryzby |
---|---|
Subject | Re: display offset along with block number in vacuum errors |
Date | |
Msg-id | 20200731215514.GC20393@telsasoft.com Whole thread Raw |
In response to | Re: display offset along with block number in vacuum errors (Mahendra Singh Thalor <mahi6run@gmail.com>) |
Responses |
Re: display offset along with block number in vacuum errors
(Justin Pryzby <pryzby@telsasoft.com>)
|
List | pgsql-hackers |
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
pgsql-hackers by date: