Re: Flush pgstats file during checkpoints

Hi,

On Fri, Jul 05, 2024 at 01:52:31PM +0900, Michael Paquier wrote:
> On Sat, Jun 29, 2024 at 11:13:04PM +0200, Tomas Vondra wrote:
> > I think those are two independent issues - knowing that the snapshot is
> > from the last checkpoint, and knowing that it's correct (not corrupted).
> > And yeah, we should be careful about fsync/durable_rename.
> 
> Yeah, that's bugging me as well.  I don't really get why we would not
> want durability at shutdown for this data.  So how about switching the
> end of pgstat_write_statsfile() to use durable_rename()?  Sounds like
> an independent change, worth on its own.
> 
> > Yeah, I was wondering about the same thing - can't this mean we fail to
> > start autovacuum? Let's say we delete a significant fraction of a huge
> > table, but then we crash before the next checkpoint. With this patch we
> > restore the last stats snapshot, which can easily have
> > 
> > n_dead_tup          | 0
> > n_mod_since_analyze | 0
> > 
> > for the table. And thus we fail to notice the table needs autovacuum.
> > AFAIK we run autovacuum on all tables with missing stats (or am I
> > wrong?). That's what's happening on replicas after switchover/failover
> > too, right?
> 
> That's the opposite, please look at relation_needs_vacanalyze().  If a
> table does not have any stats found in pgstats, like on HEAD after a
> crash, then autoanalyze is skipped and autovacuum happens only for the
> anti-wrap case.
> 
> For the database case, rebuild_database_list() uses
> pgstat_fetch_stat_dbentry() three times, discards entirely databases
> that have no stats.  Again, there should be a stats entry post a
> crash upon a reconnection.
> 
> So there's an argument in saying that the situation does not get worse
> here and that we actually may improve odds by keeping a trace of the
> previous stats, no? 

I agree, we still could get autoanalyze/autovacuum skipped due to obsolete/stales
stats being restored from the last checkpoint but that's better than having them
always skipped (current HEAD).

> In most cases, there would be a stats entry
> post-crash as an INSERT or a scan would have re-created it,

Yeap.

> but the
> stats would reflect the state registered since the last crash recovery
> (even on HEAD, a bulk delete bumping the dead tuple counter would not
> be detected post-crash).

Right.

> The case of spiky workloads may impact the 
> decision-making, of course, but at least we'd allow autovacuum to take
> some decision over giving up entirely based on some previous state of
> the stats.  That sounds like a win for me with steady workloads, less
> for bulby workloads..

I agree and it is not worst (though not ideal) that currently on HEAD.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com