Re: Conflict detection for update_deleted in logical replication - Mailing list pgsql-hackers

On Fri, Jul 18, 2025 at 5:03 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, July 18, 2025 1:25 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Jul 11, 2025 at 3:58 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Jul 10, 2025 at 6:46 PM Masahiko Sawada
> > <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Wed, Jul 9, 2025 at 9:09 PM Amit Kapila <amit.kapila16@gmail.com>
> > wrote:
> > > >
> > > > >
> > > > > > I think that even with retain_conflict_info = off, there is
> > > > > > probably a point at which the subscriber can no longer keep up
> > > > > > with the publisher. For example, if with retain_conflict_info =
> > > > > > off we can withstand 100 clients running at the same time, then
> > > > > > the fact that this performance degradation occurred with 15
> > > > > > clients explains that performance degradation is much more
> > > > > > likely to occur because of retain_conflict_info = on.
> > > > > >
> > > > > > Test cases 3 and 4 are typical cases where this feature is used
> > > > > > since the  conflicts actually happen on the subscriber, so I
> > > > > > think it's important to look at the performance in these cases.
> > > > > > The worst case scenario for this feature is that when this
> > > > > > feature is turned on, the subscriber cannot keep up even with a
> > > > > > small load, and with max_conflict_retetion_duration we enter a
> > > > > > loop of slot invalidation and re-creating, which means that
> > > > > > conflict cannot be detected reliably.
> > > > > >
> > > > >
> > > > > As per the above observations, it is less of a regression of this
> > > > > feature but more of a lack of parallel apply or some kind of
> > > > > pre-fetch for apply, as is recently proposed [1]. I feel there are
> > > > > use cases, as explained above, for which this feature would work
> > > > > without any downside, but due to a lack of some sort of parallel
> > > > > apply, we may not be able to use it without any downside for cases
> > > > > where the contention is only on a smaller set of tables. We have
> > > > > not tried, but may in cases where contention is on a smaller set
> > > > > of tables, if users distribute workload among different pub-sub
> > > > > pairs by using row filters, there also, we may also see less
> > > > > regression. We can try that as well.
> > > >
> > > > While I understand that there are some possible solutions we have
> > > > today to reduce the contention, I'm not really sure these are really
> > > > practical solutions as it increases the operational costs instead.
> > > >
> > >
> > > I assume by operational costs you mean defining the replication
> > > definitions such that workload is distributed among multiple apply
> > > workers via subscriptions either by row_filters, or by defining
> > > separate pub-sub pairs of a set of tables, right? If so, I agree with
> > > you but I can't think of a better alternative. Even without this
> > > feature as well, we know in such cases the replication lag could be
> > > large as is evident in recent thread [1] and some offlist feedback by
> > > people using native logical replication. As per a POC in the
> > > thread[1], parallelizing apply or by using some prefetch, we could
> > > reduce the lag but we need to wait for that work to mature to see the
> > > actual effect of it.
> >
> > I don't have a better alternative either.
> >
> > I agree that this feature will work without any problem when logical replication
> > is properly configured. It's a good point that update-delete conflicts can be
> > detected reliably without additional performance overhead in scenarios with
> > minimal replication lag.
> > However, this approach requires users to carefully pay particular attention to
> > replication performance and potential delays. My primary concern is that, given
> > the current logical replication performance limitations, most users who want to
> > use this feature will likely need such dedicated care for replication lag.
> > Nevertheless, most features involve certain trade-offs. Given that this is an
> > opt-in feature and future performance improvements will reduce these
> > challenges for users, it would be reasonable to have this feature at this stage.
> >
> > >
> > > The path I see with this work is to clearly document the cases
> > > (configuration) where this feature could be used without much downside
> > > and keep the default value of subscription option to enable this as
> > > false (which is already the case with the patch).
> >
> > +1
>
> Thanks for the discussion. Here is the V49 patch which includes the suggested
> doc change in 0002. I will rebase the remaining patches once the first one is
> pushed.

Thank you for updating the patch!

Here are some review comments and questions:

+   /*
+    * Do not allow users to acquire the reserved slot. This scenario may
+    * occur if the launcher that owns the slot has terminated unexpectedly
+    * due to an error, and a backend process attempts to reuse the slot.
+    */
+   if (!IsLogicalLauncher() && IsReservedSlotName(name))
+       ereport(ERROR,
+               errcode(ERRCODE_UNDEFINED_OBJECT),
+               errmsg("cannot acquire replication slot \"%s\"", name),
+               errdetail("The slot is reserved for conflict detection
and can only be acquired by logical replication launcher."));

I think it might be better to rename IsReservedSlotName() to be more
specific to the conflict detection because we might want to add more
reserved slot names in the future that would not necessarily be
acquired only by the launcher process.

---
+       if (inCommitOnly &&
+           (proc->delayChkptFlags & DELAY_CHKPT_IN_COMMIT) == 0)
+           continue;
+

I've not verified yet but is it possible that we exclude XIDs of
processes who are running on other databases?

---
Regarding the option name we've discussed:

> > The new parameter name "retain_conflict_info" sounds to me like we keep the
> > conflict information somewhere that has expired at some time such as how
> > many times insert_exists or update_origin_differs happened. How about
> > choosing a name that indicates retain dead tuples more explicitly for example
> > retain_dead_tuples?
>
> We considered the name you suggested, but we wanted to convey that this option
> not only retains dead tuples but also preserves commit timestamps and origin
> data for conflict detection, hence we opted for a more general name. Do you
> have better suggestions?

I think the commit timestamp and origin data would be retained as a
result of retaining dead tuples. While such a general name could
convey more than retaining dead tuples, I'm concerned that it could be
ambiguous what exactly to retain by the subscription. How about the
following names or something along those lines?

- retain_dead_tuples_for_conflict
- delay_vacuum_for_conflict
- keep_dead_tuples_for_conflict

---
+check_pub_conflict_info_retention(WalReceiverConn *wrconn, bool
retain_conflict_info)
+{
+   WalRcvExecResult *res;
+   Oid         RecoveryRow[1] = {BOOLOID};
+   TupleTableSlot *slot;
+   bool        isnull;
+   bool        remote_in_recovery;
+
+   if (!retain_conflict_info)
+       return;

It seems that retain_conflict_info is used only for this check to
quick exit from this function. How about calling this function only
when the caller knows retain_conflict_info is true instead of adding
it as a function argument?

---
+void
+CheckSubConflictInfoRetention(bool retain_conflict_info, bool check_guc,
+                             bool sub_disabled, int elevel_for_sub_disabled)

This function seems not to be specific to the apply workers but to
subscriptions in general. Is there any reason why we define this
function in worker.c?

---
+   if (check_guc && wal_level < WAL_LEVEL_REPLICA)
+       ereport(ERROR,
+               errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+               errmsg("\"wal_level\" is insufficient to create the
replication slot required by retain_conflict_info"),
+               errhint("\"wal_level\" must be set to \"replica\" or
\"logical\" at server start."));
+

Why does (retain_conflict_info == false && wal_level == minimal) not work?

Regards,

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