Re: error context for vacuum to include block number

On Thu, 26 Mar 2020 at 15:34, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Mar 26, 2020 at 12:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Mar 26, 2020 at 10:11 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > Seems fine.  Rather than saying "different phases" I, would say:
> > > "The index vacuum and heap vacuum phases may be called multiple times in the
> > > middle of the heap scan phase."
> > >
> >
> > Okay, I have slightly adjusted the wording as per your suggestion.
> >
> > > But actually I think the concern is not that we unnecessarily "Revert back to
> > > the old phase" but that we do it in a *loop*.  Which I agree doesn't make
> > > sense, to go back and forth between "scanning heap" and "truncating".
> > >
> >
> > Fair point.  I have moved the change to the truncate phase at the
> > caller of lazy_heap_truncate() which should address this concern.
> > Sawada-San, does this address your concern?
> >
>
> Forgot to attach the patch, doing now.

Thank you for updating the patch! The changes around
lazy_truncate_heap() looks good to me.

I have two comments;

1.
@@ -1844,9 +1914,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber
blkno, Buffer buffer,
    int         uncnt = 0;
    TransactionId visibility_cutoff_xid;
    bool        all_frozen;
+   LVRelStats  olderrcbarg;

    pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno);

+   /* Update error traceback information */
+   olderrcbarg = *vacrelstats;
+   update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_VACUUM_HEAP,
+                             blkno, NULL, false);

Since we update vacrelstats->blkno during in the loop in
lazy_vacuum_heap() we unnecessarily update blkno twice to the same
value. Also I think we don't need to revert back the callback
arguments in lazy_vacuum_page(). Perhaps we can either remove the
change of lazy_vacuum_page() or move the code updating
vacrelstats->blkno to the beginning of lazy_vacuum_page(). I prefer
the latter.

2.
+/*
+ * Update vacuum error callback for the current phase, block, and index.
+ *
+ * free_oldindname is true if the previous "indname" should be freed.
It must be
+ * false if the caller has copied the old LVRelStats, to avoid keeping a
+ * pointer to a freed allocation.  In which case, the caller should call again
+ * with free_oldindname as true to avoid a leak.
+ */
+static void
+update_vacuum_error_cbarg(LVRelStats *errcbarg, int phase, BlockNumber blkno,
+                         char *indname, bool free_oldindname)

I'm not sure why "free_oldindname" is necessary. Since we initialize
vacrelstats->indname with NULL and revert the callback arguments at
the end of functions that needs update them, vacrelstats->indname is
NULL at the beginning of lazy_vacuum_index() and lazy_cleanup_index().
And we make a copy of index name in update_vacuum_error_cbarg(). So I
think we can pfree the old index name if errcbarg->indname is not NULL.


Regards,

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