Re: Add memory_limit_hits to pg_stat_replication_slots

On Tue, Sep 23, 2025 at 1:52 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> 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(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Sep 22, 2025 at 1:41 PM Bertrand Drouvot
> > > <bertranddrouvot(dot)pg(at)gmail(dot)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.

I find "exceeded" is fine as the documentation for logical decoding
also uses it[1]:

Similar to spill-to-disk behavior, streaming is triggered when the
total amount of changes decoded from the WAL (for all in-progress
transactions) exceeds the limit defined by logical_decoding_work_mem
setting.

One comment for the v3 patch:

+ <para>
+ Number of times <literal>logical_decoding_work_mem</literal> has been
+ exceeded while decoding changes from WAL for this slot.
+ </para>

How about rewording it to like:

Number of times the memory used by logical decoding has exceeded
logical_decoding_work_mem.

Regards,

[1] https://www.postgresql.org/docs/devel/logicaldecoding-streaming.html

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com