Subject: Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber

From: Amit Kapila <[email-masked]>
Date: Wed, 21 Aug 2024 15:27:46 +0530

On Tue, Aug 20, 2024 at 2:01 PM shveta malik <[email-masked]> wrote:
>
> On Tue, Aug 20, 2024 at 11:36 AM Amit Kapila <[email-masked]> wrote:
> >
> > On Wed, Aug 14, 2024 at 10:26 AM shveta malik <[email-masked]> wrote:
> > >
> > > >
> > > > Thanks for the detailed analysis. I agree with your analysis that we
> > > > need to reset the origin information for the shutdown path to avoid it
> > > > being advanced incorrectly. However, the patch doesn't have sufficient
> > > > comments to explain why we need to reset it for both the ERROR and
> > > > Shutdown paths. Can we improve the comments in the patch?
> > > >
> > > > Also, for the ERROR path, can we reset the origin information in
> > > > apply_error_callback()?
> > >
> > > Please find v4 attached. Addressed comments in that.
> > >
> >
> > The patch looks mostly good to me. I have slightly changed a few of
> > the comments in the attached. What do you think of the attached?
> >
>
> Looks good to me. Please find backported patches attached.
>

Pushed.

-- 
With Regards,
Amit Kapila.

---

From: Tom Lane <[email-masked]>
Date: Thu, 07 Nov 2024 14:23:47 -0500

> Pushed.

I came across this commit while preparing release notes, and I'm
worried about whether it doesn't create more problems than it solves.
The intent stated in the thread subject is to prevent an apply worker
from advancing past a prepared transaction if the subscriber side
doesn't permit prepared transactions.  However, it appears to me that
the committed patch doesn't permit an apply worker to advance past
any failing transaction whatsoever.  Was any thought given to how
a DBA would get out of such a situation and get replication flowing
again?  In the prepared-xact case, it's at least clear that you
could increase max_prepared_transactions and restart the subscriber
installation.  In the general case, it's not very obvious that you'd
even know what the problem is let alone have an easy way to fix it.

In other words: I thought the original design here was to
intentionally ignore apply errors and keep going, on the theory that
that was better than blocking replication altogether.  This commit
has reversed that decision, on the strength of little or no
discussion AFAICS.  Are we really ready to push this into minor
releases of stable branches?  Is it a good idea even on HEAD?

			regards, tom lane

---

From: Amit Kapila <[email-masked]>
Date: Fri, 8 Nov 2024 08:35:38 +0530

> even know what the problem is let alone have an easy way to fix it.

This is by design, so we don't let the apply worker proceed in case of
any ERROR. For example, the apply worker keeps retrying to apply the
transaction when there is a unique key violation error while applying
(which could be due to the subscriber side having a unique key defined
but the publisher doesn't or the subscriber already has a row with the
same value). We need manual intervention to continue the replication.
To do that, she can create a subscription with the option
'disable_on_error'. Then apply worker will stop on ERROR instead of
retrying. Then, she can either manually remove a conflicting row or
use ALTER SUBSCRIPTION ... SKIP ... to get past the conflicting/error
transaction. Alternatively, the transaction can also be skipped by
calling the pg_replication_origin_advance() function. To use SKIP or
origin_advance function, in the ERROR log we print the LSN (CONTEXT:
processing remote data for replication origin "pg_16395" during
"INSERT" for replication target relation "public.test" in transaction
725 finished at 0/14C0378). She needs to use LSN value 0/14C0378 to
skip the error transaction. We have explained this in the docs [1].

> that was better than blocking replication altogether
>

No, that was not the intention because otherwise, we will silently
create inconsistency on the subscriber side.

.  This commit
> releases of stable branches?  Is it a good idea even on HEAD?

I hope the explanation above addresses your concern.

[1] - https://www.postgresql.org/docs/devel/logical-replication-conflicts.html

-- 
With Regards,
Amit Kapila.