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

On Tue, Jun 23, 2020 at 7:13 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> On Mon, Jun 22, 2020 at 01:57:12PM -0700, Andres Freund wrote:
> >
> > 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.
>

Yeah, I think this is a good point against adding a separate struct.
I also don't think that we can buy much by doing this optimization.
To me, the current code looks good in this regard.

> 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.
>

Few comments for 0002-Add-assert-and-document-why-indname-is-safe
-----------------------------------------------------------------------------------------------------
- /* Free index name from any previous phase */
if (errinfo->indname)
+ {
+ /*
+ * indname is only ever saved during lazy_vacuum_index(), which
+ * during which the phase information is not not further
+ * manipulated, until it's restored before returning from
+ * lazy_vacuum_index().
+ */
+ Assert(indname == NULL);
+
pfree(errinfo->indname);
+ errinfo->indname = NULL;
+ }

It is not very clear that this is the place where we are saving the
state. I think it would be better to do in the caller (ex. in before
statement olderrinfo = *vacrelstats; in lazy_vacuum_index()) where it
is clear that we are saving the state for later use.

I guess for the restore case we are already assigning NULL via
"errinfo->indname = indname ? pstrdup(indname) : NULL;" in
update_vacuum_error_info. I think some more comments in the function
update_vacuum_error_info would explain it better.

0001-Rename-from-errcbarg, looks fine to me but we can see if others
have any opinion on the naming (especially changing VACUUM_ERRCB* to
VACUUM_ERRINFO*).

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