On Tue, May 19, 2026 at 7:30 PM vignesh C <[email protected]> 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
