From: Justin Pryzby <pryzby@telsasoft.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.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: Tue, 24 Mar 2020 08:48:49 -0500
Message-ID: <20200324134848.GH21443@telsasoft.com> (raw)
In-Reply-To: <CAA4eK1+O8q9_e10DTOnAbX6Mf9C1bSDBZFmhCLSihb-ZgBXsGQ@mail.gmail.com>
References: <CAA4eK1+iuT4gqiqYVdLkOggjA9q5v+dVxT_G3dOyHVokn+YjBg@mail.gmail.com>
<CA+fd4k6jdiEW_+sPEMWS2TmSnE-ddAdFDSQu9WpXJsK6c2=+-Q@mail.gmail.com>
<20200323081639.GI2563@telsasoft.com>
<CAA4eK1KaWto3+4+EsLPRFL6HKs8=_42V2ivAFEPBU-o_o6AYXw@mail.gmail.com>
<20200324041616.GA21443@telsasoft.com>
<CAA4eK1J4wNO425WjKj2Yd58Ag=LovSkxoHTuBJphFma+9jLAJA@mail.gmail.com>
<CA+fd4k5TFS87WXsgne17dbYGUq79bvx-WnumfF22eqkjCHtXkA@mail.gmail.com>
<CAA4eK1+vjO-M4OaHUjjDpTkR+g47BC+mSr64qniZac6Qks_w_w@mail.gmail.com>
<CA+fd4k4ayy53qhn=rDNvazM8-rNOQ5wsAuZP05ekvvwsaed4Wg@mail.gmail.com>
<CAA4eK1+O8q9_e10DTOnAbX6Mf9C1bSDBZFmhCLSihb-ZgBXsGQ@mail.gmail.com>
On Tue, Mar 24, 2020 at 07:07:03PM +0530, Amit Kapila wrote:
> On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote:
> > 1.
> > + /* Update error traceback information */
> > + olderrcbarg = *vacrelstats;
> > + update_vacuum_error_cbarg(vacrelstats,
> > + VACUUM_ERRCB_PHASE_TRUNCATE,
> > new_rel_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);
> >
> > We need to set the error context after setting new_rel_pages.
>
> We want to cover the errors raised in count_nondeletable_pages(). In
> an earlier version of the patch, we had TRUNCATE_PREFETCH phase which
> use to cover those errors, but that was not good as we were
> setting/resetting it multiple times and it was not clear such a
> separate phase would add any value.
I insisted on covering count_nondeletable_pages since it calls ReadBuffer(),
but I think we need to at least set vacrelsats->blkno = new_rel_pages, since it
may be different, right ?
> > 2.
> > + vacrelstats->relnamespace =
> > get_namespace_name(RelationGetNamespace(onerel));
> > + vacrelstats->relname = pstrdup(RelationGetRelationName(onerel));
> >
> > I think we can pfree these two variables to avoid a memory leak during
> > vacuum on multiple relations.
>
> Yeah, I had also thought about it but I noticed that we are not
> freeing for vacrelstats. Also, I think the memory is allocated in
> TopTransactionContext which should be freed via
> CommitTransactionCommand before vacuuming of the next relation, so not
> sure if there is much value in freeing those variables.
One small reason to free them is that (as Tom mentioned upthread) it's good to
ensure that those variables are their own allocation, and not depending on
being able to access relcache or anything else during an unexpected error.
--
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, amit.kapila16@gmail.com, masahiko.sawada@2ndquadrant.com, alvherre@2ndquadrant.com, andres@anarazel.de, michael@paquier.xyz
Subject: Re: error context for vacuum to include block number
In-Reply-To: <20200324134848.GH21443@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