From: | Justin Pryzby <pryzby(at)telsasoft(dot)com> |
---|---|
To: | Mahendra Singh Thalor <mahi6run(at)gmail(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: display offset along with block number in vacuum errors |
Date: | 2020-07-27 11:13:52 |
Message-ID: | 20200727111352.GN4286@telsasoft.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Dave Page | 2020-07-27 14:57:10 | Re: Display individual query in pg_stat_activity |
Previous Message | Justin Pryzby | 2020-07-27 10:39:02 | Re: shared tempfile was not removed on statement_timeout |