Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

On Mon, Jun 22, 2020 at 01:57:12PM -0700, Andres Freund wrote:
> On 2020-06-22 15:43:11 -0500, Justin Pryzby wrote:
> > On Mon, Jun 22, 2020 at 01:09:39PM -0700, Andres Freund wrote:
> > > I'm also uncomfortable with the approach of just copying all of
> > > LVRelStats in several places:
> > >
> > > > /*
> > > > @@ -1580,9 +1648,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
> > > > int uncnt = 0;
> > > > TransactionId visibility_cutoff_xid;
> > > > bool all_frozen;
> > > > + LVRelStats olderrinfo;
> >
> > I guess the alternative is to write like
> >
> > LVRelStats olderrinfo = {
> > .phase = vacrelstats.phase,
> > .blkno = vacrelstats.blkno,
> > .indname = vacrelstats.indname,
> > };
>
> No, I don't think that's a solution. I think it's wrong to have
> something like olderrinfo in the first place. Using a struct with ~25
> members to store the current state of three variables just doesn't make
> sense. Why isn't this just a LVSavedPosition struct or something like
> that?

I'd used LVRelStats on your suggestion:
https://www.postgresql.org/message-id/20191211165425.4ewww2s5k5cafi4l%40alap3.anarazel.de
https://www.postgresql.org/message-id/20200120191305.sxi44cedhtxwr3ag%40alap3.anarazel.de

I understood the goal to be avoiding the need to add a new struct, when most
functions are already passed LVRelStats *vacrelstats.

But maybe I misunderstood. (Also, back in January, the callback was only used
for scan-heap phase, so it's increased in scope several times).

Anyway, I put together some patches for discussion purposes.

--
Justin