MIME-Version: 1.0
References: <c5e671bc-3400-c2f4-14d3-1ae06c462dad@enterprisedb.com>
 <CAA4eK1L3rxS9zyvV-SOqOTx1Rqa_9B0UxuHWX3rHfUC7z6dc6w@mail.gmail.com>
 <CAExHW5uRGwr0hAC5Gr4EbA4Q=Odjk+8nFTmdsfB1Hk-9r-81DA@mail.gmail.com>
 <CAA4eK1+pkuJhGZNA9RHOGDBUF2hhmyUr6gVCOSCTZ3wDuGbzdg@mail.gmail.com>
 <CAExHW5tR+rmrarU1m0DCmhq0e7d00XDbdhcHJV+iGkcPTfo8hg@mail.gmail.com>
 <CAA4eK1J-r8Y00CYn6WT4vexZbG5OandDrm5gcb+9Y_CCKVUkag@mail.gmail.com>
 <577ec4fc-62c5-cb4a-de0f-66dee8232d00@enterprisedb.com> <09613730-5ee9-4cc3-82d8-f089be90aa64@enterprisedb.com>
 <CA+TgmoY9tCqc_Tg4Bp8zvxvzzzDJbnopvhyVU7CwWnO21Y=Z2Q@mail.gmail.com>
 <929a38f6-b9f9-4cc4-926a-ac972e769e17@enterprisedb.com> <CA+TgmoaPTZaFTrDmvmH69Wq40RXor4VxCL99s1irfdKkMZsYjA@mail.gmail.com>
 <996e4d3c-648a-46d0-86d2-6465f53ddc58@enterprisedb.com> <CA+TgmoaSCu=bLtU7Zyth=h2Lgtzxv=xfRwLYt2ZtSKA=UN2SnQ@mail.gmail.com>
 <5bbadb71-b51e-45a1-9acc-d54f5e8850b6@enterprisedb.com> <CA+Tgmoaxertx9iuSeGOoocvq=5GTgZyXZvZBf4w24mQpyz7LRA@mail.gmail.com>
 <241d6c00-7997-480f-80c1-636b28d457cf@enterprisedb.com> <CA+TgmobO0eQ2zjekzPZ4s-iTokZ2kEeqAspuyGRyp7qFQQ4g0A@mail.gmail.com>
 <CAFiTN-s7akEaKAHqkw66dL18yW-=oWS20C1SquKsw084qJXBHw@mail.gmail.com>
 <CA+TgmoaZrNz5kb7AcSEFRDJy3yT-86qSNnu+qjs3audyEtq2zA@mail.gmail.com> <CAFiTN-syJtrgGAeE=MG5bCAmkzi4QEGPrShkBH7-LgCJfnvB6g@mail.gmail.com>
In-Reply-To: <CAFiTN-syJtrgGAeE=MG5bCAmkzi4QEGPrShkBH7-LgCJfnvB6g@mail.gmail.com>
From: Dilip Kumar <dilipbalaut@gmail.com>
Date: Wed, 21 Feb 2024 13:05:58 +0530
Message-ID: <CAFiTN-vBVTbceczNFK29-RFk8v4tXpVbtY5ap76CAjnBm3MtHA@mail.gmail.com>
Subject: Re: logical decoding and replication of sequences, take 2
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>, Amit Kapila <amit.kapila16@gmail.com>, 
	Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, 
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, 
	Masahiko Sawada <sawada.mshk@gmail.com>, 
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAFiTN-vBVTbceczNFK29-RFk8v4tXpVbtY5ap76CAjnBm3MtHA%40mail.gmail.com>
Precedence: bulk

On Tue, Feb 20, 2024 at 4:32=E2=80=AFPM Dilip Kumar <dilipbalaut@gmail.com>=
 wrote:
>
> On Tue, Feb 20, 2024 at 3:38=E2=80=AFPM Robert Haas <robertmhaas@gmail.co=
m> wrote:
>
> > Let's say fast_forward is true. Then smgr_decode() is going to skip
> > recording anything about the relfilenode, so we'll identify all
> > sequence changes as non-transactional. But look at how this case is
> > handled in seq_decode():
> >
> > + if (ctx->fast_forward)
> > + {
> > + /*
> > + * We need to set processing_required flag to notify the sequence
> > + * change existence to the caller. Usually, the flag is set when
> > + * either the COMMIT or ABORT records are decoded, but this must be
> > + * turned on here because the non-transactional logical message is
> > + * decoded without waiting for these records.
> > + */
> > + if (!transactional)
> > + ctx->processing_required =3D true;
> > +
> > + return;
> > + }
>
> It appears that the 'processing_required' flag was introduced as part
> of supporting upgrades for logical replication slots. Its purpose is
> to determine whether a slot is fully caught up, meaning that there are
> no pending decodable changes left before it can be upgraded.
>
> So now if some change was transactional but we have identified it as
> non-transaction then we will mark this flag  'ctx->processing_required
> =3D true;' so we temporarily set this flag incorrectly, but even if the
> flag would have been correctly identified initially, it would have
> been set again to true in the DecodeTXNNeedSkip() function regardless
> of whether the transaction is committed or aborted. As a result, the
> flag would eventually be set to 'true', and the behavior would align
> with the intended logic.
>
> But I am wondering why this flag is always set to true in
> DecodeTXNNeedSkip() irrespective of the commit or abort. Because the
> aborted transactions are not supposed to be replayed?  So if my
> observation is correct that for the aborted transaction, this
> shouldn't be set to true then we have a problem with sequence where we
> are identifying the transactional changes as non-transaction changes
> because now for transactional changes this should depend upon commit
> status.

I have checked this case with Amit Kapila.  So it seems in the cases
where we have sent the prepared transaction or streamed in-progress
transaction we would need to send the abort also, and for that reason,
we are setting 'ctx->processing_required' as true so that if these
WALs are not streamed we do not allow upgrade of such slots.

--=20
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com