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(dot)mshk(at)gmail(dot)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