Re: Add memory_limit_hits to pg_stat_replication_slots

On Mon, Sep 22, 2025 at 05:21:35PM +0530, shveta malik wrote:
> On Mon, Sep 22, 2025 at 4:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Sep 22, 2025 at 1:41 PM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > >
> > > >
> > > > Since other statistics counter names are camel cases I think it's
> > > > better to follow that for the new counter.
> > >
> > > Makes sense, done with memoryLimitHits in v2 attached (that's the only change
> > > as compared with v1).
> > >
> >
> > The memory_limit_hits doesn't go well with the other names in the
> > view. Can we consider memory_exceeded_count? I find
> > memory_exceeded_count (or memory_exceeds_count) more clear and
> > matching with the existing counters. Also, how about keeping it
> > immediately after slot_name in the view? Keeping it in the end after
> > total_bytes seems out of place.
> >
> 
> Since fields like spill_txns, spill_bytes, and stream_txns also talk
> about exceeding 'logical_decoding_work_mem', my preference would be to
> place this new field immediately after these spill and stream fields
> (and before total_bytes). If not this, then as Amit suggested,
> immediately before all these fields.
> Other options for name could be 'mem_limit_exceeded_count' or
> 'mem_limit_hit_count'

Thank you, Shveta and Amit, for looking at it. Since we already use txns as
abbreviation for transactions then I think it's ok to use "mem". Then I'm using
a mix of your proposals with "mem_exceeded_count" in v3 attached. Regarding the
field position, I like Shveta's proposal and did it that way.

However, technically speaking, "exceeded" is not the perfect wording since
the code was doing (and is still doing something similar to):

        if (debug_logical_replication_streaming == DEBUG_LOGICAL_REP_STREAMING_BUFFERED &&
-               rb->size < logical_decoding_work_mem * (Size) 1024)
+               !memory_limit_reached)
                return;

as the comment describes correctly using "reached":

/*
 * Check whether the logical_decoding_work_mem limit was reached, and if yes
 * pick the largest (sub)transaction at-a-time to evict and spill its changes to
 * disk or send to the output plugin until we reach under the memory limit.

So I think that "reached" or "hit" would be better wording. However, the
documentation for spill_txns, stream_txns already use "exceeded" (and not "reached")
so I went with "exceeded" for consistency. I think that's fine, if not we may want
to use "reached" for those 3 stats descriptions.

Regards,

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