MIME-Version: 1.0
References: <680bdaf6-f7d1-4536-b580-05c2760c67c6@deepbluecap.com>
 <CAA4eK1JwJw6JOnfDxtGtSRF7kM0LbEVPRmNxWeJa5+wyoG05Xg@mail.gmail.com>
 <CAA4eK1LMgqeT_bPZ3MH-VKvwOqpZyfJmF7knZhu1rqt2Pqsnwg@mail.gmail.com>
 <CAD21AoCjpj-DoASrhKCiEKuxh=0q3rC1RnAeugEwEJ=xwk=n6g@mail.gmail.com>
 <CAA4eK1LVcHD6VpH7m=8F9ru7Qdj9hsXE0gXtS93RmtTxf6L1NA@mail.gmail.com>
 <CAD21AoCeM2nni1P7Z6KXzLM=6zCdShC82sOvuvu0_hBuJkm9Qw@mail.gmail.com>
 <OS7PR01MB14968B3C263074A2DEB77DB58F565A@OS7PR01MB14968.jpnprd01.prod.outlook.com>
 <CAA4eK1+PGTdkTJwK8Necq8GrAJWP4T8cp_ponzbtogkFYvbK2w@mail.gmail.com>
 <CAD21AoCUJ=hvM=VDcH-Po=_spfyDwenniXEcgkiZU2xc4FJdJQ@mail.gmail.com>
 <CAA4eK1LqTcH5vYrCUFn=NUHv_9eYrZS6piLA3UUKD4=39xB=fQ@mail.gmail.com>
 <CAD21AoAAmGhA6NCWq=P-fzOxi4WZQCoN0y2_UOug+CMJL=YZPA@mail.gmail.com>
 <CAA4eK1LYsYCx1SNMid-4EgfYvBtxMbVvWC4q7NM7rLfyPz5GzQ@mail.gmail.com>
 <CAD21AoBDDGkHLryP1TJeh9QM3DB5ipLLThGfC6P_mk_bsxSA8A@mail.gmail.com>
 <CALDaNm0f=LgfQiV3oM3_4LOZrdLSSw8-+kUFvYNmxt4PYZymxw@mail.gmail.com>
 <CAA4eK1J8_iazK=3G76Wq6pFe+LW6FYwBOmz82Y1JTpJ8Ca1YLg@mail.gmail.com>
 <CALDaNm1-RXLA_6y_o1=j5Borz9kGapyimN=6jDbx1nDCmQzjGQ@mail.gmail.com>
 <CAD21AoCPJS3=SY28X5X_sfKzMA8PU3y0nm16ReyboFdX8=gRfg@mail.gmail.com>
 <CAA4eK1JQ2UQEbKdWUa0ozh3NdJFMJXfAKRW-vFrC3JqKxmx1FQ@mail.gmail.com>
 <CALDaNm0=JNjLqFzcAhujHLH1XQWcu+Jh_WJ6CCuqpjNb-fLnzg@mail.gmail.com>
In-Reply-To: 
 <CALDaNm0=JNjLqFzcAhujHLH1XQWcu+Jh_WJ6CCuqpjNb-fLnzg@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 2 Jun 2025 09:51:03 -0700
Message-ID: 
 <CAD21AoAe08b6smo7aq6zUUSANXpcfH2JVhU54LDyaBo59Y0DHg@mail.gmail.com>
Subject: Re: Logical replication 'invalid memory alloc request size
 1585837200' after upgrading to 17.5
To: vignesh C <vignesh21@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
 Duncan Sands <duncan.sands@deepbluecap.com>,
	"pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAD21AoAe08b6smo7aq6zUUSANXpcfH2JVhU54LDyaBo59Y0DHg%40mail.gmail.com>
Precedence: bulk

On Sun, Jun 1, 2025 at 8:13=E2=80=AFPM vignesh C <vignesh21@gmail.com> wrot=
e:
>
> On Sat, 31 May 2025 at 10:20, Amit Kapila <amit.kapila16@gmail.com> wrote=
:
> >
> > On Fri, May 30, 2025 at 11:00=E2=80=AFPM Masahiko Sawada <sawada.mshk@g=
mail.com> wrote:
> > >
> > > Thank you for updating the patch. Here are some review comments:
> > >
> > > ---
> > > +           /*
> > > +            * Make sure there's no cache pollution. Unlike the PG_TR=
Y part,
> > > +            * this must be done unconditionally because the processi=
ng 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.
> > >
> >
> > The concurrent abort handling is done for streaming and prepared
> > transactions, where we send the transaction changes to the client
> > before we read COMMIT/COMMIT PREPARED/ROLLBACK/ROLLBACK PREPARED. Now,
> > among these COMMIT/ROLLBACK PREPARED cases are handled as part of a
> > prepared transaction case. For ROLLBACK, we will never perform any
> > changes from the current transaction, so we don't need distributed
> > invalidations to be executed. For COMMIT, if we encounter any errors
> > while processing changes (this is when we reach the ERROR path, which
> > is not a concurrent abort), then we will reprocess all changes and, at
> > the end, execute both the current transaction and distributed
> > invalidations. Now, one possibility is that if, after ERROR, the
> > caller does slot_advance to skip the ERROR, then we will probably miss
> > executing the distributed invalidations, leading to data loss
> > afterwards. If the above theory is correct, then it is better to
> > execute distributed invalidation even in non-concurrent-abort cases in
> > the ERROR path.

This theory seems to be correct to me. If users re-try the logical
decoding with smaller logical_decoding_work_mem the process could end
up streaming the transaction with the wrong relcache before sending
the committed transaction?

> One possible reason this scenario may not occur is that
> pg_logical_slot_get_changes_guts uses a PG_CATCH block to handle
> exceptions, during which it calls InvalidateSystemCaches to clear the
> system cache. Because of this, I believe the scenario might not
> actually happen.
> @Sawada-san / others =E2=80=94 Are there any other cases where this could=
 still occur?

I think that we cannot confine use cases to walsender and
pg_logical_slot_get_changes_guts() given that we expose logical
decoding API at C level.

Regards,

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