On Thu, Aug 6, 2020 at 7:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Aug 5, 2020 at 12:47 AM Mahendra Singh Thalor
> <mahi6run@gmail.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?
>
I have removed this change.
> 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.
I have changed the message to what you have in the original patch.
Apart from above, I have also reset the offset number back to
InvalidOffsetNumber in lazy_scan_heap after processing all the tuples,
otherwise, it will erroneously display wring offset if any error
occurred afterward.
Let me know what you think of the changes done in the latest patch.
--
With Regards,
Amit Kapila.
