Re: WAL-logging facility for pgstats kinds - Mailing list pgsql-hackers

Hi,

On Tue, Dec 31, 2024 at 09:52:31AM +0900, Michael Paquier wrote:
> On Fri, Dec 27, 2024 at 12:32:25PM +0900, Michael Paquier wrote:
> > For clarity, the patch set has been split into several pieces, I hope
> > rather edible:
> > - 0001, a fix I've posted on a different thread [1], used in patch
> > 0004 to test this new facility.
> > - 0002, a refactoring piece to be able to expose stats kind data into
> > the frontend (for pg_waldump).
> > - 0003 is the core backend piece, introducing the WAL-logging routine
> > and the new RMGR.
> > - 0004, that provides tests for the new backend facility via a custom
> > stats kind.  Requires 0001.
> > 
> > I am going to attach it to the next CF.
> 
> 0001 has been applied as of b757abefc041.  Here is a rebased patch set
> for the rest.

Thanks for the patch!

A few random comments:

About v2-0002:

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

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

About v2-0003:

=== 2

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

=== 3

+void
+pgstat_desc(StringInfo buf, XLogReaderState *record)
+{

Looks like logicalmsg_desc(), fine by me, except:

+               /* Write data payload as a series of hex bytes */

s/data payload/stats data/ maybe?

=== 4

+const char *
+pgstat_identify(uint8 info)

Looks like logicalmsg_identify(), fine by me.

=== 5

+typedef struct xl_pgstat_data
+{
+       PgStat_Kind stats_kind;
+       size_t          data_size;              /* size of the data */
+       /* data payload, worth data_size */
+       char            data[FLEXIBLE_ARRAY_MEMBER];
+} xl_pgstat_data;

Yeah, snapshotConflictHorizon and isCatalogRel are not needed here.

=== 6

+       stats_kind = xlrec->stats_kind;
+       data_size = xlrec->data_size;
+       data = xlrec->data;
+
+       kind_info = pgstat_get_kind_info(stats_kind);
+
+       if (kind_info == NULL)
+               elog(FATAL, "pgstat_redo: invalid stats data found: kind=%u",
+                        stats_kind);
+
+       if (kind_info->redo_cb == NULL)
+               elog(FATAL, "pgstat_redo: no redo callback found: kind=%s",
+                        kind_info->name);

What about moving the data_size and data assignments after the kind_info
and kind_info->redo_cb checks?

Also,

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

About v2-0004:

=== 7

+       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).

=== 8

+       record_data.objid = entry_ref->shared_entry->key.objid;
+       record_data.entry = shfuncent->stats;

So it makes === 7 useless as here we are setting all the fields. But I think
it's good to keep === 7 as it acts as a template.

=== 9

+       if (!RecoveryInProgress() && inj_stats_wal_enabled)

s/!RecoveryInProgress()/XLogInsertAllowed()/ maybe?

Regards,

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