Re: error context for vacuum to include block number

On Tue, 24 Mar 2020 at 18:19, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 24, 2020 at 2:37 PM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Tue, 24 Mar 2020 at 13:53, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Mar 24, 2020 at 9:46 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > >
> > > > On Mon, Mar 23, 2020 at 02:25:14PM +0530, Amit Kapila wrote:
> > > > > > Yea, and it would be misleading if we reported "while scanning block..of
> > > > > > relation" if we actually failed while writing its FSM.
> > > > > >
> > > > > > My previous patches did this:
> > > > > >
> > > > > > +               case VACUUM_ERRCB_PHASE_VACUUM_FSM:
> > > > > > +                       errcontext("while vacuuming free space map of relation \"%s.%s\"",
> > > > > > +                                       cbarg->relnamespace, cbarg->relname);
> > > > > > +                       break;
> > > > > >
> > > > >
> > > > > In what kind of errors will this help?
> > > >
> > > > If there's an I/O error on an _fsm file, for one.
> > > >
> > >
> > > If there is a read or write failure, then we give error like below
> > > which already has required information.
> > > ereport(ERROR,
> > > (errcode_for_file_access(),
> > > errmsg("could not read block %u in file \"%s\": %m",
> > > blocknum, FilePathName(v->mdfd_vfd))));
> >
> > Yeah, you're right. We, however, cannot see that the error happened
> > while recording freespace or while vacuuming freespace map but perhaps
> > we can see the situation by seeing the error message in most cases. So
> > I'm okay with the current set of phases.
> >
> > I'll also test the current version of patch today.
> >
>
> okay, thanks.

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.

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.

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?

The type name ErrCbPhase seems to be very generic name, how about
VacErrCbPhase or VacuumErrCbPhase?

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services