RE: Conflict detection for update_deleted in logical replication

On Saturday, July 19, 2025 5:31 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> Here are some review comments and questions:

Thanks for the comments!

>
> + /*
> + * 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.

Agreed. I have renamed it to IsSlotForConflictCheck.

>
> ---
> 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

OK, I use the shorter version retain_conflict_info as mentioned by Amit[1].

>
> ---
> +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?

Changed as suggested.

>
> ---
> +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?

I do not have special reasons, so I moved it to subscriptioncmd.c
where most checks are performed.

>
> ---
> + 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?

I think it works because the check is skipped when rci is false. BTW, to be consistent with
check_pub_conflict_info_retention, I moved the retain_conflict_info check outside of this
function.

[1] https://www.postgresql.org/message-id/CAA4eK1JdCJK0KtV5WGYWoQVe7S3uqx-7J7t1qFpCki_rdLQFmw%40mail.gmail.com

Best Regards,
Hou zj