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
