RE: Conflict detection for update_deleted in logical replication

On Mon, Jun 16, 2025 at 7:37 PM Amit Kapila wrote:
> 
> On Thu, Jun 12, 2025 at 11:34 AM Zhijie Hou (Fujitsu) 
> <houzj.fnst@fujitsu.com> wrote:
> >
> 
> Few comments on v36 patches:
> ==========================
> 1. In advance_conflict_slot_xmin(), we first save the slot to disk, 
> then update its effective xmin, and then do the required xmin 
> computation. Now, if we don't save the slot every time, there is a 
> risk that its value can go backwards after a restart. But OTOH, for 
> physical slots maintained by walsender for physical replication, we 
> also don't save the physical slot. However, still the system works, see discussion in email: [1].
> 
> As per my understanding, even if the conflict_slot's xmin moved back 
> after restart, it shouldn't cause any problem. Because it will anyway 
> be moved ahead in the next cycle, and there won't be any rows that 
> will get removed but are required for conflict detection. If this is 
> correct, then we don't need to save the slot in advance_conflict_slot_xmin().

I think you are right, it should be OK to avoid saving the slot each time.
I have changed the patch accordingly.

> 
> 2.
> + *
> + * Issue a warning if track_commit_timestamp is not enabled when
> + * check_commit_ts is set to true.
> + *
> + * Issue a warning if the subscription is being disabled.
> + */
> +void
> +CheckSubConflictInfoRetention(bool retain_conflict_info, bool
> check_commit_ts,
> +   bool disabling_sub)
> +{
> + if (!retain_conflict_info)
> + return;
> +
> + if (check_commit_ts && !track_commit_timestamp) ereport(WARNING, 
> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("commit timestamp and origin data required for detecting
> conflicts won't be retained"),
> + errhint("Consider setting \"%s\" to true.", 
> + "track_commit_timestamp"));
> +
> + if (disabling_sub)
> + ereport(WARNING,
> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("deleted rows to detect conflicts would not be removed until
> the subscription is enabled"),
> + errhint("Consider setting %s to false.", "retain_conflict_info"));
> 
> The quoted comments atop this function just say what it is apparent 
> from the code. It is better if the comments explain why we allow to 
> proceed when the above conditions are not met.

Added.

> 
> I think we can probably add a check here that this option requires 
> wal_level = replica as the launcher needs to create a physical slot to 
> retain the required info.

Added.

After adding this ERROR, I removed the create sub (retain_conflict_info=true)
test in the subscription.sql. This is because it could cause the regression
test in 002_pg_upgrade.pl to fail as the wal_level is set to minimal in that
tap-test. I didn't add new tests since we already have tests for sub creation
in other tap tests(004_subscription.pl, 035_conflicts.pl).

Note that, the 004_subscription.pl would fail due to a crash related to commit
ca307d5. And the same has also been noticed by BF, reported in [1]. Please
ignore this error temporarily.


> 
> 3. Isn't the new check for logical slots in
> check_new_cluster_subscription_configuration() somewhat redundant with 
> the previous check done in check_new_cluster_logical_replication_slots()?
> Can't we combine both?

Merged as suggested.

> 
> Apart from this, I have made a number of changes in the comments and a 
> few other cosmetic changes in the attached.

Thanks, merged.

Here is the V38 patch set which includes the following changes:

0001:
* Addressed the comments above.
* Addressed Shveta's comments[2].

0002:
* Addressed the comments above.
* Added the document to explain that the commit timestamp data
 would not be preserved during upgrade.

0003:
Rebased

0004:
Rebased

0005:
Rebased

0006:
Rebased

0007:
Rebased

[1] https://www.postgresql.org/message-id/CALDaNm3s-jpQTe1MshsvQ8GO%3DTLj233JCdkQ7uZ6pwqRVpxAdw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAJpy0uDJ3ofFk4FFWCf6hLQZaPb%3Dry45906pqcQZ%2Bg-p3C%3D_JA%40mail.gmail.com

Best Regards,
Hou zj