Re: display offset along with block number in vacuum errors

On Sat, 25 Jul 2020 at 02:49, Mahendra Singh Thalor <mahi6run@gmail.com> 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
vacuumerror. Here, proposing a patch to add offset along with block number in vacuum errors. 
>
> In commit b61d161(Introduce vacuum errcontext to display additional information), we added vacuum errcontext to
displayadditional information(block number) so that in case of vacuum error, we can identify which block we are getting
error. Addition to block number, if we can display offset, then it will be more helpful for users. So to display
offset,here proposing two different methods(Thanks Robert for suggesting these 2 methods): 
>
> Method 1: We can report the TID as well as the block number in  errcontext.
> - errcontext("while scanning block %u of relation \"%s.%s\"",
> -   errinfo->blkno, errinfo->relnamespace, errinfo->relname);
> + errcontext("while scanning block %u and offset %u of relation \"%s.%s\"",
> +   errinfo->blkno, errinfo->offnum, errinfo->relnamespace, errinfo->relname);
>
> Above fix requires more calls to update_vacuum_error_info(). Attaching v01_0001 patch for this method.
>
> Method 2: We can improve the error messages by passing the relevant TID to heap_prepare_freeze_tuple and having it
reportthe TID as part of the error message or in the error detail. 
>   ereport(ERROR,
>   (errcode(ERRCODE_DATA_CORRUPTED),
> - errmsg_internal("found xmin %u from before relfrozenxid %u",
> + errmsg_internal("for block %u and offnum %u, found xmin %u from before relfrozenxid %u",
> + ItemPointerGetBlockNumber(tid),
> + ItemPointerGetOffsetNumber(tid),
>   xid, relfrozenxid)));
>
> Attaching v01_0002 patch for this method.
>
> Please let me know your thoughts.
>

+1 for adding offset in error messages.

I had a look at 0001 patch. You've set the vacuum error info but I
think an error won't happen during setting itemids unused:

@@ -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);
        }

        PageRepairFragmentation(page);

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services