Re: display offset along with block number in vacuum errors - Mailing list pgsql-hackers

Thanks Justin, Sawada and Michael for reviewing.

On Mon, 27 Jul 2020 at 16:43, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Fri, Jul 24, 2020 at 11:18:43PM +0530, Mahendra Singh Thalor wrote:
> > Hi hackers,
> > We discussed in another email thread[1], that it will be helpful if we can
> > display offset along with block number in vacuum error. Here, proposing a
> > patch to add offset along with block number in vacuum errors.
>
> Thanks.  I happened to see both threads, only by chance.
>
> I'd already started writing the same as your 0001, which is essentially the
> same as yours.
>
> 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 think you should also do:
>
> @@ -2976,6 +2984,7 @@ heap_page_is_all_visible(Relation rel, Buffer buf,
>                 ItemId          itemid;
>                 HeapTupleData tuple;
>
> +               vacrelstats->offset = offnum;

Agreed and fixed.

>
> I'm not sure, but maybe you'd also want to do the same in more places:
>
> @@ -2024,6 +2030,7 @@ lazy_check_needs_freeze(Buffer buf, bool *hastup)

Fixed.

> @@ -2790,6 +2797,7 @@ count_nondeletable_pages(Relation onerel, LVRelStats *vacrelstats)

I checked the code and I think there is no need to do similar changes
in count_nondeletable_pages. I will look again and will verify again.

Apart from these, I fixed comments given by Sawada and Michael in the
latest patch. Attaching v2 patch for review.

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com