Date: Sat, 1 Aug 2020 23:24:51 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Mahendra Singh Thalor <mahi6run@gmail.com>,
	pgsql-hackers@postgresql.org, Amit Kapila <akapila@postgresql.org>,
	Andres Freund <andres@anarazel.de>
Subject: Re: display offset along with block number in vacuum errors
Message-ID: <20200802042451.GJ20393@telsasoft.com>
References: 
 <CAKYtNApK488TDF4bMbw+1QH8HJf9cxdNDXquhU50TK5iv_FtCQ@mail.gmail.com>
 <20200727111352.GN4286@telsasoft.com>
 <CAKYtNApLJjAaRw0UEBBY6G1o0LRZKS7rA5n46BFh+NfwSOycdg@mail.gmail.com>
 <20200731215514.GC20393@telsasoft.com>
 <20200801061721.GF20393@telsasoft.com>
 <CAKYtNAo5O4v-cnUzgJ2x8NiR_Mp204dDi6xg-4ZT1oZZ1jFutw@mail.gmail.com>
 <CA+fd4k5Diax1-uK1LZamHiy35hpF=VPA-KpfFa_jEniAAEckeg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+fd4k5Diax1-uK1LZamHiy35hpF=VPA-KpfFa_jEniAAEckeg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Sun, Aug 02, 2020 at 01:02:42PM +0900, Masahiko Sawada wrote:
> Thank you for updating the patch!
> 
> Here are my comments on v3 patch:
> 
> @@ -2024,6 +2033,11 @@ lazy_check_needs_freeze(Buffer buf, bool *hastup)
>     if (PageIsNew(page) || PageIsEmpty(page))
>         return false;
> 
> +   /* Update error traceback information */
> +   update_vacuum_error_info(vacrelstats, &saved_err_info,
> +           VACUUM_ERRCB_PHASE_SCAN_HEAP, vacrelstats->blkno,
> +           InvalidOffsetNumber);
> +
>     maxoff = PageGetMaxOffsetNumber(page);
>     for (offnum = FirstOffsetNumber;
>          offnum <= maxoff;
> 
> You update the error callback phase to VACUUM_ERRCB_PHASE_SCAN_HEAP
> but I think we're already in that phase. I'm okay with explicitly
> setting it but on the other hand, we don't set the phase in
> heap_page_is_all_visible(). Is there any reason for that?

That part was my suggestion, so I can answer that.  I added
update_vacuum_error_info() to lazy_check_needs_freeze() to allow it to later
call restore_vacuum_error_info().

> Also, since we don't reset vacrelstats->offnum at the end of
> heap_page_is_all_visible(), if an error occurs by the end of
> lazy_vacuum_page(), the caller of  heap_page_is_all_visible(), we
> report the error context with the last offset number in the page,
> making the users confused.

So it looks like heap_page_is_all_visible() should also call the update and
restore functions.

Do you agree with my suggestion that the VACUUM phase should never try to
report an offset ?

How do you think we can phrase the message to avoid confusion due to 0-based
block number and 1-based offset ?

Thanks,
-- 
Justin