Re: Add memory_limit_hits to pg_stat_replication_slots

Hi,

On Tue, Sep 23, 2025 at 11:39:22AM -0700, Masahiko Sawada wrote:
> On Tue, Sep 23, 2025 at 1:52 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > 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.

Yes it also uses "exceeds" but I think it's not 100% accurate. It would be
if, in ReorderBufferCheckMemoryLimit, we were using "<=" instead of "<" in:

  if (debug_logical_replication_streaming == DEBUG_LOGICAL_REP_STREAMING_BUFFERED &&
       rb->size < logical_decoding_work_mem * (Size) 1024)

I think an accurate wording would be "reaches or exceeds" in all those places,
but just using "exceeds" looks good enough.

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

That sounds better, thanks! Used this wording in v4 attached (that's the only
change as compared to v3).

Regards,

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