Re: display offset along with block number in vacuum errors

On Wed, Aug 5, 2020 at 12:47 AM Mahendra Singh Thalor
<mahi6run(at)gmail(dot)com> wrote:
>
> Apart from these, I fixed Justin's comment of extra brackets(That was
> due to "patch -p 1 < file", as 002_fix was not applying directly). I
> haven't updated the document for this(Sawada's comment). I will try in
> the next patch.
> Attaching v4 patch for review.
>

Few comments on the latest patch:
1.
@@ -2640,6 +2659,7 @@ lazy_truncate_heap(Relation onerel, LVRelStats
*vacrelstats)
*/
new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
vacrelstats->blkno = new_rel_pages;
+ vacrelstats->offnum = InvalidOffsetNumber;

Do we really need to update the 'vacrelstats->offnum' here when we
have already set it to InvalidOffsetNumber in the caller?

2.
@@ -3574,8 +3605,14 @@ vacuum_error_callback(void *arg)
{
case VACUUM_ERRCB_PHASE_SCAN_HEAP:
if (BlockNumberIsValid(errinfo->blkno))
- errcontext("while scanning block %u of relation \"%s.%s\"",
- errinfo->blkno, errinfo->relnamespace, errinfo->relname);
+ {
+ if (OffsetNumberIsValid(errinfo->offnum))
+ errcontext("while scanning block %u of relation \"%s.%s\", item offset %u",
+

I am not completely sure if this error message is an improvement over
what you have in the initial version of patch "while scanning block %u
and offset %u of relation \"%s.%s\"",...". I see that Justin has
raised a concern above that whether users will be aware of 'offset'
but I also see that we have used it in a few other messages in the
code. For example:

PageIndexTupleDeleteNoCompact()
{
..
nline = PageGetMaxOffsetNumber(page);
if ((int) offnum <= 0 || (int) offnum > nline)
elog(ERROR, "invalid index offnum: %u", offnum);
..
}

hash_desc
{
..
case XLOG_HASH_INSERT:
{
xl_hash_insert *xlrec = (xl_hash_insert *) rec;

appendStringInfo(buf, "off %u", xlrec->offnum);
break;
}

Similarly in other desc functions, we have used off or offnum.

I find the message in your initial patch better.

--
With Regards,
Amit Kapila.