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