Re: shared memory stats: high level design decisions: consistency, dropping

Hi,

On 2021-03-21 14:53:50 -0700, Andres Freund wrote:
> On 2021-03-21 11:41:30 -0400, Stephen Frost wrote:
> > > 2) How to remove stats of dropped objects?
> > >
> > > [...]
> >
> > The current approach sounds pretty terrible and propagating that forward
> > doesn't seem great. Guess here I'd disagree with your gut feeling and
> > encourage trying to go 'full in', as you put it, or at least put enough
> > effort into it to get a feeling of if it's going to require a *lot* more
> > work or not and then reconsider if necessary.
>
> I think my gut's argument is that it's already a huge patch, and that
> it's better to have the the very substantial memory and disk IO savings
> with the crappy vacuum approach, than neither. And given the timeframe
> there does seem to be a substantial danger of "neither" being the
> outcome... Anyway, I'm mentally sketching out what it'd take.

I implemented this. Far from polished, but it does survive the
regression tests, including new tests in stats.sql. functions stats,
2PC and lots of naming issues (xl_xact_dropped_stats with members
ndropped and'dropped_stats) are yet to be addressed - but not
architecturally relevant.

I think there are three hairy corner-cases that I haven't thought
sufficiently about:

- It seems likely that there's a relatively narrow window where a crash
could end up not dropping stats.

The xact.c integration is basically parallel to smgrGetPendingDeletes
etc. I don't see what prevents a checkpoint from happing after
RecordTransactionCommit() (which does smgrGetPendingDeletes), but
before smgrDoPendingDeletes(). Which means that if we crash in that
window, nothing would clean up those files (and thus also not the
dropped stats that I implemented very similarly).

Closing that window seems doable (put the pending file/stat deletion
list in shared memory after logging the WAL record, but before
MyProc->delayChkpt = false, do something with that in
CheckPointGuts()), but not trivial.

If we had a good way to run pgstat_vacuum_stat() on a very
*occasional* basis via autovac, that'd be ok. But it'd be a lot nicer
if it were bulletproof.

- Does temporary table cleanup after a crash properly deal with their
stats?

- I suspect there are a few cases where pending stats in one connection
could "revive" an already dropped stat. It'd not be hard to address in
an inefficient way in the shmstats patch, but it'd come with some
efficiency penalty - but it might an irrelevant efficiency difference.

Definitely for later, but if we got this ironed out, we probably could
stop throwing stats away after crashes. Instead storing stats snapshots
alongside redo LSNs or such. It'd be nice if immediate shutdowns etc
wouldn't lead to autovacuum not doing any vacuuming for quite a while.

Greetings,

Andres Freund