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

On Mon, Jun 22, 2020 at 06:15:27PM -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > 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?
>
> That seems like rather pointless micro-optimization really; the struct's
> not *that* large. But I have a different complaint now that I look at
> this code: is it safe at all? I see that the indname field is a pointer
> to who-knows-where. If it's possible in the first place for that to
> change while this code runs, then what guarantees that we won't be
> restoring a dangling pointer to freed memory?

I'm not sure it addresses your concern, but we talked a bunch about safety
starting here:
https://www.postgresql.org/message-id/20200326150457.GB17431%40telsasoft.com
..and concluding with an explanation about CHECK_FOR_INTERRUPTS.

20200326150457(dot)GB17431(at)telsasoft(dot)com
|And I think you're right: we only save state when the calling function has a
|indname=NULL, so we never "put back" a non-NULL indname. We go from having a
|indname=NULL at lazy_scan_heap to not not-NULL at lazy_vacuum_index, and never
|the other way around. So once we've "reverted back", 1) the pointer is null;
|and, 2) the callback function doesn't access it for the previous/reverted phase
|anyway.

When this function is called by lazy_vacuum_{heap,page,index}, it's also called
a 2nd time to restore the previous phase information. When it's called the
first time by lazy_vacuum_index(), it does errinfo->indname = pstrdup(indname),
and on the 2nd call then does pfree(errinfo->indame), followed by
errinfo->indname = NULL.

|static void
|update_vacuum_error_info(LVSavedPosition *errinfo, int phase, BlockNumber blkno,
| char *indname)
|{
| errinfo->blkno = blkno;
| errinfo->phase = phase;
|
| /* Free index name from any previous phase */
| if (errinfo->indname)
| pfree(errinfo->indname);
|
| /* For index phases, save the name of the current index for the callback */
| errinfo->indname = indname ? pstrdup(indname) : NULL;
|}

If it's inadequately clear, maybe we should do:

if (errinfo->indname)
+ {
pfree(errinfo->indname);
+ Assert(indname == NULL);
+ }

--
Justin