On Thu, 26 Mar 2020 at 15:34, Amit Kapila <[email protected]> wrote:
>
> On Thu, Mar 26, 2020 at 12:03 PM Amit Kapila <[email protected]> wrote:
> >
> > On Thu, Mar 26, 2020 at 10:11 AM Justin Pryzby <[email protected]> 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
