From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Justin Pryzby <pryzby(at)telsasoft(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...) |
Date: | 2020-06-23 04:05:19 |
Message-ID: | 20200623040519.eqxkiaudueidadro@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 2020-06-22 20:43:47 -0500, Justin Pryzby wrote:
> 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).
I am only suggesting that where you save the old location, as currently
done with LVRelStats olderrinfo, you instead use a more specific
type. Not that you should pass that anywhere (except for
update_vacuum_error_info).
Greetings,
Andres Freund
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2020-06-23 04:12:09 | Re: Global snapshots |
Previous Message | Thomas Munro | 2020-06-23 04:04:36 | Re: Testing big endian code with Travis CI's new s390x support |