Re: Flush pgstats file during checkpoints - Mailing list pgsql-hackers

On 18/06/2024 9:01 am, Michael Paquier wrote:
> Hi all,
>
> On HEAD, xlog.c has the following comment, which has been on my own
> TODO list for a couple of weeks now:
>
>       * TODO: With a bit of extra work we could just start with a pgstat file
>       * associated with the checkpoint redo location we're starting from.
>
> Please find a patch series to implement that, giving the possibility
> to keep statistics after a crash rather than discard them.  I have
> been looking at the code for a while, before settling down on:
> - Forcing the flush of the pgstats file to happen during non-shutdown
> checkpoint and restart points, after updating the control file's redo
> LSN and the critical sections in the area.
> - Leaving the current before_shmem_exit() callback around, as a matter
> of delaying the flush of the stats for as long as possible in a
> shutdown sequence.  This also makes the single-user mode shutdown
> simpler.
> - Adding the redo LSN to the pgstats file, with a bump of
> PGSTAT_FILE_FORMAT_ID, cross-checked with the redo LSN.  This change
> is independently useful on its own when loading stats after a clean
> startup, as well.
> - The crash recovery case is simplified, as there is no more need for
> the "discard" code path.
> - Not using a logic where I would need to stick a LSN into the stats
> file name, implying that we would need a potential lookup at the
> contents of pg_stat/ to clean up past files at crash recovery.  These
> should not be costly, but I'd rather not add more of these.
>
> 7ff23c6d277d, that has removed the last call of CreateCheckPoint()
> from the startup process, is older than 5891c7a8ed8f, still it seems
> to me that pgstats relies on some areas of the code that don't make
> sense on HEAD (see locking mentioned at the top of the write routine
> for example).  The logic gets straight-forward to think about as
> restart points and checkpoints always run from the checkpointer,
> implying that pgstat_write_statsfile() is already called only from the
> postmaster in single-user mode or the checkpointer itself, at
> shutdown.
>
> Attached is a patch set, with the one being the actual feature, with
> some stuff prior to that:
> - 0001 adds the redo LSN to the pgstats file flushed.
> - 0002 adds an assertion in pgstat_write_statsfile(), to check from
> where it is called.
> - 0003 with more debugging.
> - 0004 is the meat of the thread.
>
> I am adding that to the next CF.  Thoughts and comments are welcome.
> Thanks,
> --
> Michael

Hi Michael.

I am working mostly on the same problem - persisting pgstat state in 
Neon (because of separation of storage and compute it has no local files).
I have two questions concerning this PR and the whole strategy for 
saving pgstat state between sessions.

1. Right now pgstat.stat is discarded after abnormal Postgres 
termination. And in your PR we are storing LSN in pgstat.staty to check 
that it matches checkpoint redo LSN. I wonder if having outdated version 
of pgstat.stat  is worse than not having it at all? Comment in xlog.c 
says: "When starting with crash recovery, reset pgstat data - it might 
not be valid." But why it may be invalid? We are writing it first to 
temp file and then rename. May be fsync() should be added here and 
durable_rename() should be used instead of rename(). But it seems to be 
better than loosing statistics. And should not add some significant 
overhead (because it happens only at shutdown). In your case we are 
checking LSN of pgstat.stat file. But once again - why it is better to 
discard file than load version from previous checkpoint?

2. Second question is also related with 1). So we saved pgstat.stat on 
checkpoint, then did some updates and then Postgres crashed. As far as I 
understand with your patch we will successfully restore pgstats.stat 
file. But it is not actually up-to-date: it doesn't reflect information 
about recent updates. If it was so critical to keep pgstat.stat 
up-to-date, then why do we allow to restore state on most recent checkpoint?

Thanks,
Konstantin