Re: Add memory_limit_hits to pg_stat_replication_slots - Mailing list pgsql-hackers

Hi Evan,

On Fri, Sep 26, 2025 at 02:34:58PM +0800, Chao Li wrote:
> Hi Bertrand,
> 
> Thanks for the patch. The patch overall goods look to me. Just a few small comments:

Thanks for looking at it!

> > On Sep 25, 2025, at 18:17, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> 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
thesame style, so that it would be easy to add new metrics in future.
 

I'm not sure: for the moment we have only this stat related to logical_decoding_work_mem,
memory usage. If we add other stats in this area later, we could add a comment
"section" as you suggest.

> 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-orderedcolumns.
 

I think it's good to have the function fields "ordering" matching the view
fields ordering. FWIW, the ordering has been discussed in [1].

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

It's already done that way in other parts of the documentation:

$ git grep "has exceeded" "*sgml"
doc/src/sgml/maintenance.sgml:    vacuum has exceeded the defined insert threshold, which is defined as:
doc/src/sgml/monitoring.sgml:        logical decoding to decode changes from WAL has exceeded
doc/src/sgml/monitoring.sgml:        from WAL for this slot has exceeded
doc/src/sgml/monitoring.sgml:        Number of times the memory used by logical decoding has exceeded
doc/src/sgml/ref/create_subscription.sgml:          retention duration has exceeded the

So that looks ok to me (I'm not a native English speaker though).

[1]: https://www.postgresql.org/message-id/CAJpy0uBskXMq65rvWm8a-KR7cSb_sZH9CPRCnWAQrTOF5fciGw%40mail.gmail.com

Regards,

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