Re: Proposal: Conflict log history table for Logical Replication

Hi Vignesh.

I checked the latest v37-0001/0002 patches combined.

My only comment is below.

======

1.
+/*
+ * drop_conflict_log_table
+ * Drop the conflict log table associated with a subscription.
+ *
+ * The conflict log table is registered as an internal dependency of the
+ * subscription. This function removes the dependency by performing a
+ * cascading deletion on the subscription object, which in turn drops the
+ * associated conflict log table.
+ *
+ * This is used to clean up conflict log tables that are no longer required,
+ * preventing accumulation of stale or orphaned relations.
+ *
+ * NOTE:
+ * Only conflict log tables are currently managed via this internal dependency
+ * mechanism. If additional internal dependencies are introduced in future,
+ * this function may require refinement to avoid unintended deletions.
+ */
+void
+drop_conflict_log_table(Oid subid, char *subname, Oid subconflictlogrelid)
+{
+ ObjectAddress object;
+ char *conflictrelname;
+
+ conflictrelname = get_rel_name(subconflictlogrelid);
+
+ ObjectAddressSet(object, SubscriptionRelationId, subid);
+ performDeletion(&object, DROP_CASCADE,
+ PERFORM_DELETION_INTERNAL |
+ PERFORM_DELETION_SKIP_ORIGINAL);
+
+ ereport(NOTICE,
+ errmsg("dropped conflict log table \"%s\" for subscription \"%s\"",
+ get_qualified_objname(PG_CONFLICT_NAMESPACE, conflictrelname),
+ subname));
+}
+

IIUC, this is a function that drops the subscription dependencies via
cascade. Since the CLT happens to be the only such dependency, it gets
dropped.

The current implementation feels backwards to me. IMO, this is really
a subscription function, so it should be refactored to be called
something like 'drop_subscription_dependencies', and not be in the
conflicts.c file. Refactoring/renaming to what it *really* does means
you won't need to give the other warnings like "may require refinement
to avoid unintended deletions". Maybe the callers do not need to be
guarded anymore -- this code can check internally so that it only does
anything when there is a known CLT associated with the subscription.

Also, the function comment should make it clearer that
PERFORM_DELETION_SKIP_ORIGINAL means the parent subscription object is
not deleted.

======
Kind Regards,
Peter Smith.
Fujitsu Australia