Re: error context for vacuum to include block number

On Mon, Mar 30, 2020 at 9:56 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Mar 30, 2020 at 02:31:53PM +0530, Amit Kapila wrote:
> > The v37-0003-Avoid-some-calls-to-RelationGetRelationName.patch looks
> > good to me.  I have added the commit message in the patch.
>
> I realized the 0003 patch has an error in lazy_vacuum_index; it should be:
>
> -                                       RelationGetRelationName(indrel),
> +                                       vacrelstats->indname,
>

Hmm, it is like that in the patch I have sent yesterday.  Are you
referring to the patch I have sent yesterday or some older version?
One thing I have noticed is that there is some saving by using
vacrelstats->relnamespace as that avoids sys cache lookup.  OTOH,
using vacrelstats->relname doesn't save much, but maybe for the sake
of consistency, we can use it.

> That was maybe due to originally using a separate errinfo for each phase, with
> one "char *relname" and no "char *indrel".
>
> > I don't think the above change is correct.  How will vacrelstats have
> > correct values when vacuum_one_index is called via parallel workers
> > (via parallel_vacuum_main)?
>
> You're right: parallel main's vacrelstats was added by this patchset and only
> the error context fields were initialized.  I fixed it up in the attached by
> also setting vacrelstats->new_rel_tuples and old_live_tuples.  It's not clear
> if this is worth it just to save an argument to two functions?
>

Right, it is not clear to me whether that is an improvement, so I
suggest let's leave that patch for now.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com