Nathan Bossart <nathandbossart@gmail.com> writes:
> Hm. I can't get excited about checking pgstat_fetch_consistency (as
> proposed in that other report), but I see that commit 02502c1bca added the
> freeing behavior in question. I wonder if it makes sense to just skip
> freeing when relation_needs_vacanalyze() is called from the view, i.e., not
> an autovacuum worker. On the other hand, maybe we shouldn't be caching
> entries for a view like this that looks through all tables in the
> database...
<carp>
Oh. I'm not happy that any part of autovacuum.c is now reachable from
SQL: that's the sort of modularity violation that will bite us on the
ass (indeed just did). Aside from this problem, the elog's that
relation_needs_vacanalyze emits seem 100% inappropriate and misleading
when it's being called from the view.
</carp>
I think perhaps the right way forward is to rethink the API
guarantees for pgstat_fetch_stat_tabentry_ext, as I speculated
about in 02502c1bc:
Note: pfree'ing the PgStat_StatTabEntry structs here seems a bit
risky, because pgstat_fetch_stat_tabentry_ext does not guarantee
anything about whether its result is long-lived. It appears okay
so long as autovacuum forces PGSTAT_FETCH_CONSISTENCY_NONE, but
I think that API could use a re-think.
I didn't want to do any such thing in a bug fix that needed to be
back-patched, but I see no reason we couldn't redefine that API
for v19. Plausible alternatives:
1. Always return a freshly palloc'd struct. Potentially adds
cycles, adds risk of a leak if caller forgets to pfree.
2. Add a "bool *should_free" parameter, like we have in tuplestores
and some other places. It's on the caller to pfree if should_free
gets set, but since we'd have to touch every caller, we'd not miss
any.
3. Add a "bool please_palloc" parameter, signaling the caller's
intent to pfree. Probably has no advantage over #2 though.
regards, tom lane
