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

On Fri, Oct 3, 2025 at 11:48 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Oct 3, 2025 at 9:26 AM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Fri, Oct 3, 2025 at 6:45 PM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Oct 03, 2025 at 05:19:42PM +0530, Ashutosh Bapat wrote:
> > > > + bool memory_limit_reached = (rb->size >= logical_decoding_work_mem *
> > > > (Size) 1024);
> > > > +
> > > > + if (memory_limit_reached)
> > > > + rb->memExceededCount += 1;
> > >
> > > Thanks for looking at it!
> > >
> > > > If the memory limit is hit but no transaction was serialized, the
> > > > stats won't be updated since UpdateDecodingStats() won't be called. We
> > > > need to call UpdateDecodingStats() in ReorderBufferCheckMemoryLimit()
> > > > if no transaction was streamed or spilled.
> > >
> > > I did some testing and the stats are reported because UpdateDecodingStats() is
> > > also called in DecodeCommit(), DecodeAbort() and DecodePrepare() (in addition
> > > to ReorderBufferSerializeTXN() and ReorderBufferStreamTXN()). That's also why
> > > ,for example, total_txns is reported even if no transaction was streamed or
> > > spilled.
> >
> > In a very pathological case, where all transactions happen to be
> > aborted while decoding and yet memory limit is hit many times, nothing
> > will be reported till first committed transaction after it is decoded.
> > Which may never happen. I didn't find a call stack where by
> > UpdateDecodingStats could be reached from
> > ReorderBufferCheckAndTruncateAbortedTXN().
>
> The more we report the status frequently, the less chances we lose the
> statistics in case of logical decoding being interrupted but the more
> overheads we have to update the statistics. I personally prefer not to
> call UpdateDecodingStats() frequently since pgstat_report_replslot()
> always flush the statistics. If the transaction is serialized or
> streamed, we can update the memExceededCount together with other
> statistics such as streamBytes and spillBytes. But if we can free
> enough memory only by truncating already-aborted transactions, we need
> to rely on the next committed/aborted/prepared transaction to update
> the statistics. So how about calling UpdateDecodingStats() only in
> case where we only truncate aborted transactions and the memory usage
> gets lower than the limit?

Yes, that's what my intention was.

>
> I've attached the patch that implements this idea with a small
> refactoring. It also has the change to the regression test results
> we've discussed.

The change looks good to me.

Given Andres's comment, in a nearby thread, about being cautious about
adding useless statistics, I think this one needs a bit more
discussion. In the proposal email Bertant wrote
> Please find attached a patch to $SUBJECT, to report the number of times the
> logical_decoding_work_mem has been reached.
>
> With such a counter one could get a ratio like total_txns/memory_limit_hits.
>
> That could help to see if reaching logical_decoding_work_mem is rare or
> frequent enough. If frequent, then maybe there is a need to adjust
> logical_decoding_work_mem.
>

I agree with the goal that we need a metric to decide whether
exceeding logical decoding work mem is frequent or not.

> Based on my simple example above, one could say that it might be possible to get
> the same with:
>
> (spill_count - spill_txns) + (stream_count - stream_txns)
>
> but that doesn't appear to be the case with a more complicated example (277 vs 247):
>
>   slot_name   | spill_txns | spill_count | total_txns | stream_txns | stream_count | memory_limit_hits |
(spc-spct)+(strc-strt)
>
--------------+------------+-------------+------------+-------------+--------------+-------------------+------------------------
>  logical_slot |        405 |         552 |         19 |           5 |          105 |               277 |
     247 
> (1 row)

Agreed that any arithmetic on the currently reported counters don't
provide the exact number of times the memory limit was hit. The
question is whether there exists some arithmetic which gives a good
indicator of whether hitting memory limit is frequent or rare. In your
example, is the difference 247 vs 277 significant enough to lead to
the wrong conclusion about the frequency? Is there another case where
this difference is going to lead to a wrong conclusion?

--
Best Wishes,
Ashutosh Bapat