From: Justin Pryzby <pryzby@telsasoft.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Mahendra Singh Thalor <mahi6run@gmail.com>
Cc: pgsql-hackers@postgresql.org, Amit Kapila <akapila@postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Subject: Re: display offset along with block number in vacuum errors
Date: Sat, 1 Aug 2020 23:24:51 -0500
Message-ID: <20200802042451.GJ20393@telsasoft.com> (raw)
In-Reply-To: <CA+fd4k5Diax1-uK1LZamHiy35hpF=VPA-KpfFa_jEniAAEckeg@mail.gmail.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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, masahiko.sawada@2ndquadrant.com, mahi6run@gmail.com, akapila@postgresql.org, andres@anarazel.de
Subject: Re: display offset along with block number in vacuum errors
In-Reply-To: <20200802042451.GJ20393@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox