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

Justin Pryzby <pryzby(at)telsasoft(dot)com> writes:
> On Mon, Jun 22, 2020 at 06:15:27PM -0400, Tom Lane wrote:
>> 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.

If we're relying on that, I'd replace the "save" action by an Assert that
indname is NULL, and the "restore" action by just assigning NULL again.
That eliminates all concern about whether the restored value is valid.

regards, tom lane