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

On Saturday, July 19, 2025 5:31 AM Masahiko Sawada <sawada.mshk@gmail.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