Re: Flush pgstats file during checkpoints

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