Thread: Flush pgstats file during checkpoints

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

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

On 6/28/24 09:32, Konstantin Knizhnik wrote:
> 
> 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?
> 

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.

> 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?
> 

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?

It'd not be such an issue if we updated stats during recovery, but I
think think we're doing that. Perhaps we should, which might also help
on replicas - no idea if it's feasible, though.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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?  In most cases, there would be a stats entry
post-crash as an INSERT or a scan would have re-created it, 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).  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..

> It'd not be such an issue if we updated stats during recovery, but I
> think think we're doing that. Perhaps we should, which might also help
> on replicas - no idea if it's feasible, though.

Stats on replicas are considered an independent thing AFAIU (scans are
counted for example in read-only queries).  If we were to do that we
may want to split stats handling between nodes in standby state and
crash recovery.  Not sure if that's worth the complication.  First,
the stats exist at node-level.
--
Michael

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.

Please find attached a rebased patch set with the durability point
addressed in 0001.  There were also some conflicts.

Note that I have applied the previous 0002 adding an assert in
pgstat_write_statsfile() as 734c057a8935, as I've managed to break
again this assumption while hacking more on this area..
--
Michael

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

Hi,

On Fri, Jul 12, 2024 at 03:42:21PM +0900, Michael Paquier wrote:
> 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.
> 
> Please find attached a rebased patch set with the durability point
> addressed in 0001.  There were also some conflicts.

Thanks!

Looking at 0001:

+               /* error logged already */

Maybe mention it's already logged by durable_rename() (like it's done in
InstallXLogFileSegment(), BaseBackup() for example).

Except this nit, 0001 LGTM.

Need to spend more time and thoughts on 0002+.

Regards,

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

Hi,

On Fri, Jul 12, 2024 at 12:10:26PM +0000, Bertrand Drouvot wrote:
> Need to spend more time and thoughts on 0002+.

I think there is a corner case, say:

1. shutdown checkpoint at LSN1
2. startup->reads the stat file (contains LSN1)->all good->read stat file and
remove it
3. crash (no checkpoint occured between 2. and 3.) 
4. startup (last checkpoint is still LSN1)->no stat file (as removed in 2.) 

In that case we start with empty stats.

Instead of removing the stat file, should we keep it around until the first call
to pgstat_write_statsfile()?

Regards,

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

On Fri, Jul 12, 2024 at 01:01:19PM +0000, Bertrand Drouvot wrote:
> Instead of removing the stat file, should we keep it around until the first call
> to pgstat_write_statsfile()?

Oops.  You are right, I have somewhat missed the unlink() once we are
done reading the stats file with a correct redo location.
--
Michael

On Fri, Jul 12, 2024 at 12:10:26PM +0000, Bertrand Drouvot wrote:
> Looking at 0001:
>
> +               /* error logged already */
>
> Maybe mention it's already logged by durable_rename() (like it's done in
> InstallXLogFileSegment(), BaseBackup() for example).
>
> Except this nit, 0001 LGTM.

Tweaked the comment, and applied 0001 for durable_rename().  Thanks
for the review.
--
Michael

On Tue, Jul 16, 2024 at 10:37:39AM +0900, Michael Paquier wrote:
> On Fri, Jul 12, 2024 at 01:01:19PM +0000, Bertrand Drouvot wrote:
>> Instead of removing the stat file, should we keep it around until the first call
>> to pgstat_write_statsfile()?
>
> Oops.  You are right, I have somewhat missed the unlink() once we are
> done reading the stats file with a correct redo location.

The durable_rename() part has been applied.  Please find attached a
rebase of the patch set with all the other comments addressed.
--
Michael

Hi,

On Wed, Jul 17, 2024 at 12:52:12PM +0900, Michael Paquier wrote:
> On Tue, Jul 16, 2024 at 10:37:39AM +0900, Michael Paquier wrote:
> > On Fri, Jul 12, 2024 at 01:01:19PM +0000, Bertrand Drouvot wrote:
> >> Instead of removing the stat file, should we keep it around until the first call
> >> to pgstat_write_statsfile()?
> >
> > Oops.  You are right, I have somewhat missed the unlink() once we are
> > done reading the stats file with a correct redo location.
> 
> The durable_rename() part has been applied.  Please find attached a
> rebase of the patch set with all the other comments addressed.

Thanks!

Looking at 0001:

1 ===

-               pgstat_write_statsfile();
+               pgstat_write_statsfile(GetRedoRecPtr());

Not related with your patch but this comment in the GetRedoRecPtr() function:

     * grabbed a WAL insertion lock to read the authoritative value in
     * Insert->RedoRecPtr

sounds weird. Should'nt that be s/Insert/XLogCtl/?

2 ===

+       /* Write the redo LSN, used to cross check the file loaded */

Nit: s/loaded/read/?

3 ===

+       /*
+        * Read the redo LSN stored in the file.
+        */
+       if (!read_chunk_s(fpin, &file_redo) ||
+               file_redo != redo)
+               goto error;

I wonder if it would make sense to have dedicated error messages for
"file_redo != redo" and for "format_id != PGSTAT_FILE_FORMAT_ID". That would
ease to diagnose as to why the stat file is discarded.

Looking at 0002:

LGTM

Looking at 0003:

4 ===

@@ -5638,10 +5634,7 @@ StartupXLOG(void)
         * 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.
         */
-       if (didCrash)
-               pgstat_discard_stats();
-       else
-               pgstat_restore_stats(checkPoint.redo);
+       pgstat_restore_stats(checkPoint.redo)

remove the TODO comment?

5 ===

+ * process) if the stats file has a redo LSN that matches with the .

unfinished sentence?

6 ===

- * Should only be called by the startup process or in single user mode.
+ * This is called by the checkpointer or in single-user mode.
  */
 void
-pgstat_discard_stats(void)
+pgstat_flush_stats(XLogRecPtr redo)
 {

Would that make sense to add an Assert in pgstat_flush_stats()? (checking what 
the above comment states).

Regards,

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

On Mon, Jul 22, 2024 at 07:01:41AM +0000, Bertrand Drouvot wrote:
> 1 ===
> Not related with your patch but this comment in the GetRedoRecPtr() function:
>
>      * grabbed a WAL insertion lock to read the authoritative value in
>      * Insert->RedoRecPtr
>
> sounds weird. Should'nt that be s/Insert/XLogCtl/?

No, the comment is right.  We are retrieving a copy of
Insert->RedoRecPtr here.

> 2 ===
>
> +       /* Write the redo LSN, used to cross check the file loaded */
>
> Nit: s/loaded/read/?

WFM.

> 3 ===
>
> +       /*
> +        * Read the redo LSN stored in the file.
> +        */
> +       if (!read_chunk_s(fpin, &file_redo) ||
> +               file_redo != redo)
> +               goto error;
>
> I wonder if it would make sense to have dedicated error messages for
> "file_redo != redo" and for "format_id != PGSTAT_FILE_FORMAT_ID". That would
> ease to diagnose as to why the stat file is discarded.

Yep.  This has been itching me quite a bit, and that's a bit more than
just the format ID or the redo LSN: it relates to all the read_chunk()
callers.  I've taken a shot at this with patch 0001, implemented on
top of the rest.  Adjusted as well the redo LSN read to have more
error context, now in 0002.

> Looking at 0003:
>
> 4 ===
>
> @@ -5638,10 +5634,7 @@ StartupXLOG(void)
>          * 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.
>          */
> -       if (didCrash)
> -               pgstat_discard_stats();
> -       else
> -               pgstat_restore_stats(checkPoint.redo);
> +       pgstat_restore_stats(checkPoint.redo)
>
> remove the TODO comment?

Pretty sure I've removed that more than one time already, and that
this is a rebase accident.  Thanks for noticing.

> 5 ===
>
> + * process) if the stats file has a redo LSN that matches with the .
>
> unfinished sentence?

This is missing a reference to the control file.

> 6 ===
>
> - * Should only be called by the startup process or in single user mode.
> + * This is called by the checkpointer or in single-user mode.
>   */
>  void
> -pgstat_discard_stats(void)
> +pgstat_flush_stats(XLogRecPtr redo)
>  {
>
> Would that make sense to add an Assert in pgstat_flush_stats()? (checking what
> the above comment states).

There is one in pgstat_write_statsfile(), not sure there is a point in
duplicating the assertion in both.

Attaching a new v4 series, with all these comments addressed.
--
Michael

Hi,

On Tue, Jul 23, 2024 at 12:52:11PM +0900, Michael Paquier wrote:
> On Mon, Jul 22, 2024 at 07:01:41AM +0000, Bertrand Drouvot wrote:
> > 3 ===
> > 
> > +       /*
> > +        * Read the redo LSN stored in the file.
> > +        */
> > +       if (!read_chunk_s(fpin, &file_redo) ||
> > +               file_redo != redo)
> > +               goto error;
> > 
> > I wonder if it would make sense to have dedicated error messages for
> > "file_redo != redo" and for "format_id != PGSTAT_FILE_FORMAT_ID". That would
> > ease to diagnose as to why the stat file is discarded.
> 
> Yep.  This has been itching me quite a bit, and that's a bit more than
> just the format ID or the redo LSN: it relates to all the read_chunk()
> callers.  I've taken a shot at this with patch 0001, implemented on
> top of the rest.

Thanks! 0001 attached is v4-0001-Revert-Test-that-vacuum-removes-tuples-older-than.patch
so I guess you did not attached the right one.

> Attaching a new v4 series, with all these comments addressed.

Thanks!

Looking at 0002:

1 ===

   if (!read_chunk(fpin, ptr, info->shared_data_len))
+  {
+        elog(WARNING, "could not read data of stats kind %d for entry of type %c",
+                kind, t);

Nit: what about to include the "info->shared_data_len" value in the WARNING?

2 ===

   if (!read_chunk_s(fpin, &name))
+  {
+         elog(WARNING, "could not read name of stats kind %d for entry of type %c",
+                kind, t);
        goto error;
+  }
   if (!pgstat_is_kind_valid(kind))
+  {
+       elog(WARNING, "invalid stats kind %d for entry of type %c",
+                kind, t);
        goto error;
+  }

Shouldn't we swap those 2 tests so that we check that the kind is valid right
after this one?

  if (!read_chunk_s(fpin, &kind))
+ {
+      elog(WARNING, "could not read stats kind for entry of type %c", t);
       goto error;
+ }

Looking at 0003: LGTM

Looking at 0004: LGTM

Regards,

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

On Mon, Jul 29, 2024 at 04:46:17AM +0000, Bertrand Drouvot wrote:
> Thanks! 0001 attached is v4-0001-Revert-Test-that-vacuum-removes-tuples-older-than.patch
> so I guess you did not attached the right one.

I did attach the right set of patches, please ignore 0001 entirely:
the patch series is made of three patches, beginning with 0002 :)

> Looking at 0002:
>
>    if (!read_chunk(fpin, ptr, info->shared_data_len))
> +  {
> +        elog(WARNING, "could not read data of stats kind %d for entry of type %c",
> +                kind, t);
>
> Nit: what about to include the "info->shared_data_len" value in the WARNING?

Good idea, so added.

>    if (!read_chunk_s(fpin, &name))
> +  {
> +         elog(WARNING, "could not read name of stats kind %d for entry of type %c",
> +                kind, t);
>         goto error;
> +  }
>    if (!pgstat_is_kind_valid(kind))
> +  {
> +       elog(WARNING, "invalid stats kind %d for entry of type %c",
> +                kind, t);
>         goto error;
> +  }
>
> Shouldn't we swap those 2 tests so that we check that the kind is valid right
> after this one?

Hmm.  We could, but this order is not buggy either.  So I've let it
as-is for now, just adding the WARNINGs.

By the way, I have noticed an extra path where a WARNING would not be
logged while playing with corrupted pgstats files: when the entry type
itself is incorrect.  I have added an extra elog() in this case, and
applied 0001.  Err..  0002, sorry ;)

> Looking at 0003: LGTM
> Looking at 0004: LGTM

Thanks.  Attached are the two remaining patches, for now.  I'm
planning to get back to these in a few days.
--
Michael

Hi,

On Tue, Jul 30, 2024 at 03:25:31PM +0900, Michael Paquier wrote:
> On Mon, Jul 29, 2024 at 04:46:17AM +0000, Bertrand Drouvot wrote:
> > Thanks! 0001 attached is v4-0001-Revert-Test-that-vacuum-removes-tuples-older-than.patch
> > so I guess you did not attached the right one.
> 
> I did attach the right set of patches, please ignore 0001 entirely:
> the patch series is made of three patches, beginning with 0002 :)

Yeah, saw that ;-)

> > Looking at 0002:
> >
> >    if (!read_chunk(fpin, ptr, info->shared_data_len))
> > +  {
> > +        elog(WARNING, "could not read data of stats kind %d for entry of type %c",
> > +                kind, t);
> > 
> > Nit: what about to include the "info->shared_data_len" value in the WARNING?
> 
> Good idea, so added.

Thanks!

> > Looking at 0003: LGTM
> > Looking at 0004: LGTM
> 
> Thanks.  Attached are the two remaining patches, for now.  I'm
> planning to get back to these in a few days.

Did a quick check and still LGTM.

Regards,

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

On Tue, Jul 30, 2024 at 08:53:48AM +0000, Bertrand Drouvot wrote:
> Did a quick check and still LGTM.

Applied 0003 for now to add the redo LSN to the pgstats file, adding
the redo LSN to the two DEBUG2 entries when reading and writing while
on it, that I forgot.  (It was not 01:57 where I am now.)

Attached is the last one.
--
Michael

Hi,

On Mon, Aug 26, 2024 at 01:56:40PM +0900, Michael Paquier wrote:
> On Fri, Aug 02, 2024 at 02:11:34AM +0900, Michael Paquier wrote:
> > Applied 0003 for now to add the redo LSN to the pgstats file, adding
> > the redo LSN to the two DEBUG2 entries when reading and writing while
> > on it, that I forgot.  (It was not 01:57 where I am now.)
> > 
> > Attached is the last one.
> 
> The CF bot has been complaining in injection_points as an effect of
> the stats remaining after a crash, so rebased to adapt to that.

Thanks!

Checking the V7 diffs as compared to V4:

1. In pgstat_write_statsfile():

-    elog(DEBUG2, "writing stats file \"%s\"", statfile);
+    elog(DEBUG2, "writing stats file \"%s\" with redo %X/%X", statfile,
+         LSN_FORMAT_ARGS(redo));

2. and the ones in injection_points/t/001_stats.pl:

 +# On crash the stats are still there.
  $node->stop('immediate');
  $node->start;
  $numcalls = $node->safe_psql('postgres',
      "SELECT injection_points_stats_numcalls('stats-notice');");
 -is($numcalls, '', 'number of stats after crash');
 +is($numcalls, '3', 'number of stats after crash');
  $fixedstats = $node->safe_psql('postgres',
      "SELECT * FROM injection_points_stats_fixed();");
 -is($fixedstats, '0|0|0|0|0', 'fixed stats after crash');
 +is($fixedstats, '1|0|2|1|1', 'fixed stats after crash');

They both LGTM.

Regards,

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

>> It'd not be such an issue if we updated stats during recovery, but I
>> think think we're doing that. Perhaps we should, which might also help
>> on replicas - no idea if it's feasible, though.
> 
> Stats on replicas are considered an independent thing AFAIU (scans are
> counted for example in read-only queries).  If we were to do that we
> may want to split stats handling between nodes in standby state and
> crash recovery.  Not sure if that's worth the complication.  First,
> the stats exist at node-level.

Hmm, I'm a bit disappointed this doesn't address replication. It makes 
sense that scans are counted separately on a standby, but it would be 
nice if stats like last_vacuum were propagated from primary to standbys. 
  I guess that can be handled separately later.


Reviewing v7-0001-Flush-pgstats-file-during-checkpoints.patch:

There are various race conditions where a stats entry can be leaked in 
the pgstats file. I.e. relation is dropped, but its stats entry is 
retained in the stats file after crash. In the worst case, suck leaked 
entries can accumulate until the stats file is manually removed, which 
resets all stats again. Perhaps that's acceptable - it's still possible 
leak the actual relation file for a new relation on crash, after all, 
which is much worse (I'm glad Horiguchi-san is working on that [1]).

For example:
1. BEGIN; CREATE TABLE foo (); ANALYZE foo;
2. CHECKPOINT;
3. pg_ctl restart -m immediate

This is the same scenario where we leak the relfile, but now you can 
have it with e.g. function statistics too.

Until 5891c7a8ed, there was a mechanism to garbage collect such orphaned 
entries (pgstat_vacuum()). How bad would it be to re-introduce that? Or 
can we make it more watertight so that there are no leaks?


If you do this:

pg_ctl start -D data
pg_ctl stop -D data -m immediate
pg_ctl start -D data
pg_ctl stop -D data -m immediate

You get this in the log:

2024-09-02 16:28:37.874 EEST [1397281] WARNING:  found incorrect redo 
LSN 0/160A3C8 (expected 0/160A440)

I think it's failing to flush the stats file at the end of recovery 
checkpoint.


[1] 
https://www.postgresql.org/message-id/20240901.010925.656452225144636594.horikyota.ntt%40gmail.com

-- 
Heikki Linnakangas
Neon (https://neon.tech)