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
