Re: error context for vacuum to include block number - Mailing list pgsql-hackers

On Wed, 4 Mar 2020 at 04:32, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Feb 27, 2020 at 09:09:42PM -0300, Alvaro Herrera wrote:
> > > +           case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
> > > +                   if (BlockNumberIsValid(cbarg->blkno))
> > > +                           errcontext("while vacuuming block %u of relation \"%s.%s\"",
> > > +                                           cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> > > +                   break;
> >
> > I think you should still call errcontext() when blkno is invalid.
>
> In my experience while testing, the conditional avoids lots of CONTEXT noise
> from interrupted autovacuum, at least.  I couldn't easily reproduce it with the
> current patch, though, maybe due to less pushing and popping.
>
> > Maybe it would make sense to make the LVRelStats struct members be char
> > arrays rather than pointers.  Then you memcpy() or strlcpy() them
> > instead of palloc/free.
>
> I had done that in the v15 patch, to allow passing it to parallel workers.
> But I don't think it's really needed.
>
> On Tue, Mar 03, 2020 at 10:05:42PM +0900, Masahiko Sawada wrote:
> > I was concerned about fsm vacuum; vacuum error context might show heap
> > scan while actually doing fsm vacuum. But perhaps we can update
> > callback args for that. That would be helpful for user to distinguish
> > that the problem seems to be either in heap vacuum or in fsm vacuum.
>
> Done in the attached.  But I think non-error reporting of additional progress
> phases is out of scope for this patch.

Thank you for updating the patch. But we have two more places where we
do fsm vacuum.

            /*
             * Periodically do incremental FSM vacuuming to make newly-freed
             * space visible on upper FSM pages.  Note: although we've cleaned
             * the current block, we haven't yet updated its FSM entry (that
             * happens further down), so passing end == blkno is correct.
             */
            if (blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
            {
                FreeSpaceMapVacuumRange(onerel, next_fsm_block_to_vacuum,
                                        blkno);
                next_fsm_block_to_vacuum = blkno;
and

    /*
     * Vacuum the remainder of the Free Space Map.  We must do this whether or
     * not there were indexes.
     */
    if (blkno > next_fsm_block_to_vacuum)
        FreeSpaceMapVacuumRange(onerel, next_fsm_block_to_vacuum, blkno);


---
 static void vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats,
                             LVShared *lvshared, LVSharedIndStats
*shared_indstats,
-                            LVDeadTuples *dead_tuples);
+                            LVDeadTuples *dead_tuples, LVRelStats
*vacrelstats);
 static void lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
-                             LVDeadTuples *dead_tuples, double reltuples);
+                             LVDeadTuples *dead_tuples, double
reltuples, LVRelStats *vacrelstats);

These functions have LVDeadTuples and LVRelStats but LVDeadTuples can
be referred by LVRelStats. If we want to use LVRelStats as callback
argument, we can remove function arguments that can be referred by
LVRelStats.

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/



PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services