From: Justin Pryzby <pryzby@telsasoft.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org
Subject: Re: error context for vacuum to include block number
Date: Thu, 26 Mar 2020 10:04:57 -0500
Message-ID: <20200326150457.GB17431@telsasoft.com> (raw)
In-Reply-To: <CA+fd4k61uPzeAWT1EZOwPWq7DkhgVTrC_wmUNCzVW+MbP9Wjfg@mail.gmail.com>
References: <CAA4eK1KRvWEpcvEM9rc_oNi8PFSps-kOvzebxFUE0JXdanjE-g@mail.gmail.com>
<20200325101229.GR21443@telsasoft.com>
<CAA4eK1KwcMWa7Jwewh48xC9o9pExzd=CuRuox94+AHvYr3F7Qg@mail.gmail.com>
<CA+fd4k6-zFG=mvkZzgsLQZ3v+df5Bk_0d8tpSzcRV0u40dM0rw@mail.gmail.com>
<20200325124155.GU21443@telsasoft.com>
<CAA4eK1+=ToXurzjOcPyQ4j6Vz2nG2C-cUdg=yZoUpN+g1k5m6w@mail.gmail.com>
<20200326044115.GB28385@telsasoft.com>
<CAA4eK1LBtp+qpsz8uR-oj2h6jzhLEVo9TEdm46anKM-n=8nS0g@mail.gmail.com>
<CAA4eK1L33gvQ1z-CHctzTWng2HfmFn0cDJif_mpBOvG47Mymow@mail.gmail.com>
<CA+fd4k61uPzeAWT1EZOwPWq7DkhgVTrC_wmUNCzVW+MbP9Wjfg@mail.gmail.com>
On Thu, Mar 26, 2020 at 08:56:54PM +0900, Masahiko Sawada wrote:
> 1.
> @@ -1844,9 +1914,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber
> blkno, Buffer buffer,
> int uncnt = 0;
> TransactionId visibility_cutoff_xid;
> bool all_frozen;
> + LVRelStats olderrcbarg;
>
> pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno);
>
> + /* Update error traceback information */
> + olderrcbarg = *vacrelstats;
> + update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_VACUUM_HEAP,
> + blkno, NULL, false);
>
> Since we update vacrelstats->blkno during in the loop in
> lazy_vacuum_heap() we unnecessarily update blkno twice to the same
> value. Also I think we don't need to revert back the callback
> arguments in lazy_vacuum_page(). Perhaps we can either remove the
> change of lazy_vacuum_page() or move the code updating
> vacrelstats->blkno to the beginning of lazy_vacuum_page(). I prefer
> the latter.
We want the error callback to be in place during lazy_scan_heap, since it
calls ReadBufferExtended().
We can't remove the change in lazy_vacuum_page, since it's also called from
lazy_scan_heap, if there are no indexes.
We want lazy_vacuum_page to "revert back" since we go from "scanning heap" to
"vacuuming heap". lazy_vacuum_page was the motivation for saving and restoring
the called arguments, otherwise lazy_scan_heap() would have to clean up after
the function it called, which was unclean. Now, every function cleans up after
itself.
Does that address your comment ?
> +static void
> +update_vacuum_error_cbarg(LVRelStats *errcbarg, int phase, BlockNumber blkno,
> + char *indname, bool free_oldindname)
>
> I'm not sure why "free_oldindname" is necessary. Since we initialize
> vacrelstats->indname with NULL and revert the callback arguments at
> the end of functions that needs update them, vacrelstats->indname is
> NULL at the beginning of lazy_vacuum_index() and lazy_cleanup_index().
> And we make a copy of index name in update_vacuum_error_cbarg(). So I
> think we can pfree the old index name if errcbarg->indname is not NULL.
We want to avoid doing this:
olderrcbarg = *vacrelstats // saves a pointer
update_vacuum_error_cbarg(... NULL); // frees the pointer and sets indname to NULL
update_vacuum_error_cbarg(... olderrcbarg.oldindnam) // puts back the pointer, which has been freed
// hit an error, and the callback accesses the pfreed pointer
I think that's only an issue for lazy_vacuum_index().
And I think you're right: we only save state when the calling function has a
indname=NULL, so we never "put back" a non-NULL indname. We go from having a
indname=NULL at lazy_scan_heap to not not-NULL at lazy_vacuum_index, and never
the other way around. So once we've "reverted back", 1) the pointer is null;
and, 2) the callback function doesn't access it for the previous/reverted phase
anyway.
Hm, I was just wondering what happens if an error happens *during*
update_vacuum_error_cbarg(). It seems like if we set
errcbarg->phase=VACUUM_INDEX before setting errcbarg->indname=indname, then an
error would cause a crash. And if we pfree and set indname before phase, it'd
be a problem when going from an index phase to non-index phase. So maybe we
have to set errcbarg->phase=VACUUM_ERRCB_PHASE_UNKNOWN while in the function,
and errcbarg->phase=phase last.
--
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, amit.kapila16@gmail.com, alvherre@2ndquadrant.com, andres@anarazel.de, michael@paquier.xyz
Subject: Re: error context for vacuum to include block number
In-Reply-To: <20200326150457.GB17431@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