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

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.

Regards,
Vignesh