On Tue, 24 Mar 2020 at 22:37, Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Tue, Mar 24, 2020 at 6:18 PM Masahiko Sawada
> <masahiko.saw...@2ndquadrant.com> wrote:
> >
> >
> > I've read through the latest version patch (v31), here are my comments:
> >
> > 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 got the point. But if we set the error context before that, I think
we need to change the error context message. The error context message
of heap truncation phase is "while truncating relation \"%s.%s\" to %u
blocks", but cbarg->blkno will be the number of blocks of the current
relation.
case VACUUM_ERRCB_PHASE_TRUNCATE:
if (BlockNumberIsValid(cbarg->blkno))
errcontext("while truncating relation \"%s.%s\" to %u blocks",
cbarg->relnamespace, cbarg->relname, cbarg->blkno);
break;
>
> > 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.
Right, thank you for explanation. I was concerned memory leak because
relation name and schema name could be large compared to vacrelstats
but I agree to free them at commit time.
>
> > 3.
> > +/* Phases of vacuum during which we report error context. */
> > +typedef enum
> > +{
> > + VACUUM_ERRCB_PHASE_UNKNOWN,
> > + VACUUM_ERRCB_PHASE_SCAN_HEAP,
> > + VACUUM_ERRCB_PHASE_VACUUM_INDEX,
> > + VACUUM_ERRCB_PHASE_VACUUM_HEAP,
> > + VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
> > + VACUUM_ERRCB_PHASE_TRUNCATE
> > +} ErrCbPhase;
> >
> > Comparing to the vacuum progress phases, there is not a phase for
> > final cleanup which includes updating the relation stats. Is there any
> > reason why we don't have that phase for ErrCbPhase?
> >
>
> I think we can add it if we want, but it was not clear to me if that is
> helpful.
Yeah, me too. The most likely place where an error happens is
vac_update_relstats but the error message seems to be enough.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
