On 4/3/21 9:42 PM, Alvaro Herrera wrote:
> Thanks for the quick rework. I like this design much better and I think
> this is pretty close to committable. Here's a rebased copy with some
> small cleanups (most notably, avoid calling pgstat_propagate_changes
> when the partition doesn't have a tabstat entry; also, free the lists
> that are allocated in a couple of places).
>
> I didn't actually verify that it works.
>
Yeah, this approach seems much simpler, I think. That being said, I
think there's a couple issues:
1) I still don't understand why inheritance and declarative partitioning
are treated differently. Seems unnecessary nad surprising, but maybe
there's a good reason?
2) pgstat_recv_tabstat
Should it really reset changes_since_analyze_reported in both branches?
AFAICS if the "found" branch does this
tabentry->changes_since_analyze_reported = 0;
that means we lose the counter any time tabstats are received, no?
That'd be wrong, because we'd propagate the changes repeatedly.
3) pgstat_recv_analyze
Shouldn't it propagate the counters before resetting them? I understand
that for the just-analyzed relation we can't do better, but why not to
propagate the counters to parents? (Not necessarily from this place in
the stat collector, maybe the analyze process should do that.)
4) pgstat_recv_reportedchanges
I think this needs to be more careful when updating the value - the
stats collector might have received other messages modifying those
counters (including e.g. PGSTAT_MTYPE_ANALYZE with a reset), so maybe we
can get into situation with
(changes_since_analyze_reported > changes_since_analyze)
if we just blindly increment the value. I'd bet would lead to funny
stuff. So maybe this should be careful to never exceed this?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
