Re: Add memory_limit_hits to pg_stat_replication_slots

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?

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.

Regards,

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