On Mon, Jun 22, 2020 at 06:15:27PM -0400, Tom Lane wrote: > Andres Freund <and...@anarazel.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.gb17...@telsasoft.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