Hi Bertrand, Thanks for the patch. The patch overall goods look to me. Just a few small comments:
> On Sep 25, 2025, at 18:17, Bertrand Drouvot <[email protected]> > wrote: > > <v5-0001-Add-mem_exceeded_count-to-pg_stat_replication_slo.patch> 1. ``` --- a/src/include/replication/reorderbuffer.h +++ b/src/include/replication/reorderbuffer.h @@ -690,6 +690,9 @@ struct ReorderBuffer int64 streamCount; /* streaming invocation counter */ int64 streamBytes; /* amount of data decoded */ + /* Number of times logical_decoding_work_mem has been reached */ + int64 memExceededCount; ``` For other metrics, the commented with “Statistics about xxx” above, and line comment after every metric. Maybe use the same style, so that it would be easy to add new metrics in future. 2. ``` --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -2100,7 +2100,7 @@ pg_stat_get_archiver(PG_FUNCTION_ARGS) Datum pg_stat_get_replication_slot(PG_FUNCTION_ARGS) { -#define PG_STAT_GET_REPLICATION_SLOT_COLS 10 +#define PG_STAT_GET_REPLICATION_SLOT_COLS 11 text *slotname_text = PG_GETARG_TEXT_P(0); NameData slotname; TupleDesc tupdesc; @@ -2125,11 +2125,13 @@ pg_stat_get_replication_slot(PG_FUNCTION_ARGS) INT8OID, -1, 0); TupleDescInitEntry(tupdesc, (AttrNumber) 7, "stream_bytes", INT8OID, -1, 0); - TupleDescInitEntry(tupdesc, (AttrNumber) 8, "total_txns", + TupleDescInitEntry(tupdesc, (AttrNumber) 8, "mem_exceeded_count", INT8OID, -1, 0); - TupleDescInitEntry(tupdesc, (AttrNumber) 9, "total_bytes", + TupleDescInitEntry(tupdesc, (AttrNumber) 9, "total_txns", INT8OID, -1, 0); - TupleDescInitEntry(tupdesc, (AttrNumber) 10, "stats_reset", + TupleDescInitEntry(tupdesc, (AttrNumber) 10, "total_bytes", + INT8OID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 11, "stats_reset", TIMESTAMPTZOID, -1, 0); BlessTupleDesc(tupdesc); ``` Is it better to add the new field in the last place? Say if a client does “select * from pg_stat_get_replication_slit()”, it will just gets an extra column instead of mis-ordered columns. 3. ``` + <para> + Number of times the memory used by logical decoding has exceeded + <literal>logical_decoding_work_mem</literal>. + </para> ``` Feels like “has” is not needed. Maybe the wording can be simplified as: Number of times logical decoding exceeded <literal>logical_decoding_work_mem</literal>. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
