Re: Proposal: Conflict log history table for Logical Replication

On Tue, May 19, 2026 at 7:30 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> Rest of the comments are handled, the attached v36 version patches
> have the changes for the same.
> Also the comment from [1] has been fixed in this version.
>

Thanks Vignesh.

A few comments for 0001 and 002 combined (I merged them and reviewed
for ease of review)

1)

+ * IsConflictLogTableClass
+ * True iff namespace is pg_conflict.
+ *
+ * Does not perform any catalog accesses.
*/
bool
-IsConflictClass(Form_pg_class reltuple)
+IsConflictLogTableClass(Form_pg_class reltuple)

I think this function is trying to find if the reltuple is a CLT
rather than namepspace is pg_conflict.
We should change this comment. See IsToastRelation, IsToastClass.

Suggestion:
True iff Form_pg_class tuple represents a subscription-specific
Conflict Log Table.

2)

Both DropSubscription and AlterSubscription has below code to drop CLT:

+ if (OidIsValid(subconflictlogrelid))
+ {
+ char *conflictrelname = get_rel_name(subconflictlogrelid);
+
+ /*
+ * Conflict log tables are recorded as internal dependencies of the
+ * subscription. We must drop the dependent objects before the
+ * subscription itself is removed. By using
+ * PERFORM_DELETION_SKIP_ORIGINAL, we ensure that only the conflict log
+ * table is reaped while the subscription remains for the final
+ * deletion step.
+ */
+ 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));
+ }

Why don't we create a function
drop_conflict_log_table(subconflictlogrelid) and use it both places.

3)
+++ b/src/backend/commands/subscriptioncmds.c

+#include "catalog/heap.h"
+#include "catalog/pg_am_d.h"

It compiles now without these inclusion. 002 should remove these as well.

4)
AlterSubscription:
+ bool want_table = (opts.conflictlogdest == CONFLICT_LOG_DEST_TABLE ||
+ opts.conflictlogdest == CONFLICT_LOG_DEST_ALL);
+ bool has_oldtable = (old_dest == CONFLICT_LOG_DEST_TABLE ||
+ old_dest == CONFLICT_LOG_DEST_ALL);

Shall we replace checks at both places with CONFLICTS_LOGGED_TO_TABLE

~~

003,004: No comments

~~

Reviewing further.

thanks
Shveta