Re: WAL-logging facility for pgstats kinds

On Tue, Dec 31, 2024 at 11:29:26AM +0000, Bertrand Drouvot wrote:
> === 1
> 
> + *     Copyright (c) 2001-2024, PostgreSQL Global Development Group
> 
> As pgstat_kind.h is a new file, s/Copyright (c) 2001-2024/Copyright (c) 2025/
> maybe?

Fixed.

> No more comments, as v2-0002 is "just" moving some stuff from pgstat.h to
> pgstat_kind.h.

Thanks.  This split is independent of the main topic of the thread,
and I think that it just makes the declarations cleaner, so I'm
planning to apply this one.  I was also planning to write some custom
tool to manipulate the pgstats file.  That will help a bit.

For the rest of the patch set, let's see how the discussion moves on..

> Same as === 1 for pgstatdesc.c, pgstat_xlog.c and pgstat_xlog.h.

Fixed.

> +void
> +pgstat_desc(StringInfo buf, XLogReaderState *record)
> +{
> 
> Looks like logicalmsg_desc(), fine by me, except:

Yes, there's not much you can do here, except if we invent one day a
way to load external libraries in the frontend.

> s/data payload/stats data/ maybe?
> s/invalid stats data/invalid stats kind/ maybe?

Okay.

> +       PgStat_StatInjRecord record_data = {0};
> 
> PgStat_StatInjRecord does not contain any padding but as it acts as a template,
> better to use memset() instead? (to cover the cases where the record contains
> padding).

Dammit, forgot about that.  Good point about the padding, especially
for the template bits.

> +       if (!RecoveryInProgress() && inj_stats_wal_enabled)
> 
> s/!RecoveryInProgress()/XLogInsertAllowed()/ maybe?

I'd rather use RecoveryInProgress() here even if XLogInsertAllowed()
is a synonym of that, minus the update of LocalXLogInsertAllowed for
the local process.
--
Michael