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 <pgsql-hackers@postgresql.org>
Subject: Re: error context for vacuum to include block number
Date: Wed, 25 Mar 2020 07:41:55 -0500
Message-ID: <20200325124155.GU21443@telsasoft.com> (raw)
In-Reply-To: <CA+fd4k6-zFG=mvkZzgsLQZ3v+df5Bk_0d8tpSzcRV0u40dM0rw@mail.gmail.com>
References: <CA+fd4k6KdmyLqMc3GC5QGX9d9WPmDWnUugEfJ7xhyLQRBEmrRA@mail.gmail.com>
<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>
On Wed, Mar 25, 2020 at 09:27:44PM +0900, Masahiko Sawada wrote:
> On Wed, 25 Mar 2020 at 20:24, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Mar 25, 2020 at 3:42 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > Attached patch addressing these.
> > >
> >
> > Thanks, you forgot to remove the below declaration which I have
> > removed in attached.
> >
> > @@ -724,20 +758,20 @@ lazy_scan_heap(Relation onerel, VacuumParams
> > *params, LVRelStats *vacrelstats,
> > PROGRESS_VACUUM_MAX_DEAD_TUPLES
> > };
> > int64 initprog_val[3];
> > + ErrorContextCallback errcallback;
> >
> > Apart from this, I have ran pgindent and now I think it is in good
> > shape. Do you have any other comments? Sawada-San, can you also
> > check the attached patch and let me know if you have any additional
> > comments.
> >
>
> Thank you for updating the patch! I have a question about the following code:
>
> + /* Update error traceback information */
> + olderrcbarg = *vacrelstats;
> + update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_TRUNCATE,
> + vacrelstats->nonempty_pages, NULL, false);
> +
> /*
> * Scan backwards from the end to verify that the end pages actually
> * contain no tuples. This is *necessary*, not optional, because
> * other backends could have added tuples to these pages whilst we
> * were vacuuming.
> */
> new_rel_pages = count_nondeletable_pages(onerel, vacrelstats);
> + vacrelstats->blkno = new_rel_pages;
>
> if (new_rel_pages >= old_rel_pages)
> {
> /* can't do anything after all */
> UnlockRelation(onerel, AccessExclusiveLock);
> return;
> }
>
> /*
> * Okay to truncate.
> */
> RelationTruncate(onerel, new_rel_pages);
>
> + /* Revert back to the old phase information for error traceback */
> + update_vacuum_error_cbarg(vacrelstats,
> + olderrcbarg.phase,
> + olderrcbarg.blkno,
> + olderrcbarg.indname,
> + true);
>
> vacrelstats->nonempty_pages is the last non-empty block while
> new_rel_pages, the result of count_nondeletable_pages(), is the number
> of blocks that we can truncate to in this attempt. Therefore
> vacrelstats->nonempty_pages <= new_rel_pages. This means that we set a
> lower block number to arguments and then set a higher block number
> after count_nondeletable_pages, and then revert it back to
> VACUUM_ERRCB_PHASE_SCAN_HEAP phase and the number of blocks of
> relation before truncation, after RelationTruncate(). It can be
> repeated until no more truncating can be done. Why do we need to
> revert back to the scan heap phase? If we can use
> vacrelstats->nonempty_pages in the error context message as the
> remaining blocks after truncation I think we can update callback
> arguments once at the beginning of lazy_truncate_heap() and don't
> revert to the previous phase, and pop the error context after exiting.
Perhaps. We need to "revert back" for the vacuum phases, which can be called
multiple times, but we don't need to do that here.
In the future, if we decided to add something for final cleanup phase (say),
it's fine (and maybe better) to exit truncate_heap() without resetting the
argument, and we'd immediately set it to CLEANUP.
I think the same thing applies to lazy_cleanup_index, too. It can be called
from a parallel worker, but we never "go back" to a heap scan.
--
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: <20200325124155.GU21443@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