RE: Conflict detection for update_deleted in logical replication

On Friday, August 8, 2025 2:34 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Thu, Aug 7, 2025 at 10:10 AM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > On Tuesday, August 5, 2025 10:09 AM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > Here is V57 patch set which addressed most of comments.
> > >
> > > In this version, I also fixed a bug that the apply worker continued
> > > to find dead tuples even if it has already stop retaining dead tuples.
> >
> > Here is a V58 patch set which improved few things by internal review:
> >
> > 0001:
> >
>
> Thank You for the patches, please find a few comments on 001 alone:

Thanks for the comments.

>
> 1)
> + /*
> + * Return if the wait time has not exceeded the maximum limit
> + * (max_conflict_retention_duration).
> + */
> + if (!TimestampDifferenceExceeds(rdt_data->candidate_xid_time, now,
> + max_conflict_retention_duration +
> + rdt_data->table_sync_wait_time))
>
> We can add comments here as in why we are adding table-sync time to
> max_conflict_retention_duration.

Added.

>
> 2)
> relmutex comment says:
>
> /* Used for initial table synchronization. */
> Oid relid;
> char relstate;
> XLogRecPtr relstate_lsn;
> slock_t relmutex;
>
> We shall update this comment as now we are using it for other purposes. Also
> name is specific to relation (due to originally created for table-sync case). We
> can rename it to be more general so that it can be used for oldest-xid access
> purposes as well.

Changed the name and added comments.

>
> 3)
> + Assert(TransactionIdIsValid(rdt_data->candidate_xid));
> + Assert(rdt_data->phase == RDT_WAIT_FOR_PUBLISHER_STATUS ||
> + rdt_data->phase == RDT_WAIT_FOR_LOCAL_FLUSH);
> +
> + if (!max_conflict_retention_duration)
> + return false;
>
> Shall we move 'max_conflict_retention_duration' NULL check as the first step.
> Or do you think it will be better to move it to the caller before
> should_stop_conflict_info_retention is invoked?

I think these Asserts are good to have, even if the GUC is not
specified, so I kept the current style.

>
> 4)
> + The information useful for conflict detection is no longer retained if
> + all apply workers associated with the subscriptions, where
> + <literal>retain_dead_tuples</literal> is enabled, confirm that the
> + retention duration exceeded the
> + <literal>max_conflict_retention_duration</literal>. To re-enable
> + retention, you can disable <literal>retain_dead_tuples</literal> and
> + re-enable it after confirming this replication slot has been dropped.
>
> But the replication slot will not be dropped unless all the subscriptions have
> disabled retain_dead_tuples. So shall the above doc somehow mention this
> part as well otherwise it could be misleading for users.

Added.

> 5)
> pg_stat_subscription_stats: retain_dead_tuples
>
> Can it cause confusion as both subscription's parameter and
> pg_stat_subscription_stats's column have the same name while may have
> different values. Shall the stats one be named as
> 'effective_retain_dead_tuples'?

I think the prefix "effective_" is typically used for non-boolean options (such
as effective_cache_size or effective_io_concurrency). So, I opted for the name
"dead_tuple_retention_active" as it aligns with some existing names like
"row_security_active."

Here is V59 patch set which addressed above comments in 0001.

Best Regards,
Hou zj