Re: display offset along with block number in vacuum errors

Thanks Justin, Sawada and Michael for reviewing.

On Mon, 27 Jul 2020 at 16:43, Justin Pryzby <pryzby(at)telsasoft(dot)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