Re: Logical replication 'invalid memory alloc request size 1585837200' after upgrading to 17.5

On Sat, 31 May 2025 at 13:27, vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 30 May 2025 at 23:00, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Thank you for updating the patch. Here are some review comments:
> >
> > @@ -3439,9 +3464,27 @@ ReorderBufferAddInvalidations(ReorderBuffer
> > *rb, TransactionId xid,
> >                               XLogRecPtr lsn, Size nmsgs,
> >                               SharedInvalidationMessage *msgs)
> >  {
> > -   ReorderBufferTXN *txn;
> > +   ReorderBufferAddInvalidationsExtended(rb, xid, lsn, nmsgs, msgs, false);
> > +}
> >
> > If the patch is the changes for master do we need to have an extended
> > version of ReorderBufferAddInvalidation()?
>
> This has been removed now and ReorderBufferAddDistributedInvalidtions
> has been added
>
> > ---
> > +           /*
> > +            * Make sure there's no cache pollution. Unlike the PG_TRY part,
> > +            * this must be done unconditionally because the processing might
> > +            * fail before we reach invalidation messages.
> > +            */
> > +           if (rbtxn_inval_all_cache(txn))
> > +               InvalidateSystemCaches();
> > +           else
> > +               ReorderBufferExecuteInvalidations(txn->ninvalidations_distr,
> > +
> > txn->distributed_invalidations);
> > +
> >
> > If we don't need to execute the distributed inval message in an error
> > path other than detecting concurrent abort, we should describe the
> > reason.
>
> Removed it to keep it in the common error path
>
> > ---
> > Given that we don't account the memory usage of both
> > txn->invalidations and txn->distributed_invalidations, probably we can
> > have a lower limit, say 8MB (or lower?), to avoid memory exhaustion.
>
> Modified
>
> > ---
> > +   if ((for_inval && !AllocSizeIsValid(req_mem_size)) ||
> > +       rbtxn_inval_all_cache(txn))
> >     {
> > -       txn->ninvalidations = nmsgs;
> > -       txn->invalidations = (SharedInvalidationMessage *)
> > -           palloc(sizeof(SharedInvalidationMessage) * nmsgs);
> > -       memcpy(txn->invalidations, msgs,
> > -              sizeof(SharedInvalidationMessage) * nmsgs);
> > +       txn->txn_flags |= RBTXN_INVAL_ALL_CACHE;
> > +
> > +       if (*invalidations)
> > +       {
> > +           pfree(*invalidations);
> > +           *invalidations = NULL;
> > +           *ninvalidations = 0;
> > +       }
> >
> > RBTXN_INVAL_ALL_CACHE seems to have an effect only on the distributed
> > inval messages. One question is do we need to care about the overflow
> > of txn->invalidations as well? If no, does it make sense to have a
> > separate function like ReorderBufferAddDistributedInvalidtions()
> > instead of having an extended version of
> > ReorderBufferAddInvalidations()? Some common routines can also be
> > declared as a static function if needed.
>
> Modified
>
> The attached v7 version patch has the changes for the same.

Here is the patch, including updates for the back branches.
The main difference from master is that the newly added structure
members are appended at the end in the back branches to preserve
compatibility. The invalidation addition logic remains consistent with
master: a new function, ReorderBufferAddDistributedInvalidations, has
been introduced to handle distributed invalidations. Shared logic
between ReorderBufferAddInvalidations and
ReorderBufferAddDistributedInvalidations has been factored out into a
common helper, ReorderBufferAddInvalidationsCommon. This approach
simplifies future merges and, in my assessment, does not introduce any
backward compatibility issues.

Regards,
Vignesh