Date: Mon, 27 Jul 2020 06:13:52 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Mahendra Singh Thalor <mahi6run@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: display offset along with block number in vacuum errors
Message-ID: <20200727111352.GN4286@telsasoft.com>
References: 
 <CAKYtNApK488TDF4bMbw+1QH8HJf9cxdNDXquhU50TK5iv_FtCQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAKYtNApK488TDF4bMbw+1QH8HJf9cxdNDXquhU50TK5iv_FtCQ@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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;

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;

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)
@@ -2790,6 +2797,7 @@ count_nondeletable_pages(Relation onerel, LVRelStats *vacrelstats)

-- 
Justin