Re: Proposal: Conflict log history table for Logical Replication

Hi Vignesh,

Some review comments for v38-0001/0002 combined.

======
src/backend/commands/subscriptioncmds.c

AlterSubscriptionConflictLogDestination:

1.
* Update the conflict log table associated with a subscription when its
* conflict log destination is changed.

Somehow, that 'its' sounded awkward to me.

SUGGESTION.
When the subscription's 'conflict_log_destination' is changed, update
the conflict log table if required.

~~~

2.
+ * If the new destination requires a conflict log table and none was previously
+ * required, this function validates an existing conflict log table identified
+ * by the subscription specific naming convention or creates a new one.

What does this mean: "validates an existing conflict log table". How
is there an "existing" CLT when you already said "none was previously
required". Maybe this needs more explanation. If it is talking about
"not already associated with another subscription", then it should
just say that.

Anyway, it seems validation that the comment claims this function is
doing is not done here at all, but is really done by
'create_conflict_log_table'.

~~~

3.
+static bool
+AlterSubscriptionConflictLogDestination(Subscription *sub,
+ ConflictLogDest logdest,
+ Oid *conflicttablerelid)

3a.
There was no forward declaration of this static function, but there
was for all the others.

~

3b.
Static functions should use snake-case names.

~~~

4.
Personally, I think it is more natural to read LEFT-TO-RIGHT,
OLD-THEN-NEW, etc., so I felt that the has_oldtable check should
always come before want_table.

Also, the 'ifs' seemed tricky because it's not obvious what
has/need_table combinations are missing. e.g. The following seems
easier to me. And probably lots of comments could be moved to here in
the code as well, instead of in the function comment.

SUGGESTION
if (has_old_table)
{
/* There is a CLT already. */

if (!want_table)
{
/* Remove it. */
drop_subscription_dependencies(sub->oid, sub->name, sub->conflictlogrelid);
update_relid = true;
}
}
else
{
/* There was no previous CLT. */

if (want_table)
{
/* Create one. */
relid = create_conflict_log_table(sub->oid, sub->name, sub->owner);
update_relid = true;
}
}

~~~

5.
+static void
+drop_subscription_dependencies(Oid subid, char *subname,
+ Oid subconflictlogrelid)
+{
+ ObjectAddress object;
+ char *conflictrelname;
+
+ conflictrelname = get_rel_name(subconflictlogrelid);
+
+ /*
+ * By using PERFORM_DELETION_SKIP_ORIGINAL, we ensure that only the
+ * conflict log table is deleted while the subscription remains.
+ */
+ 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));
+}
+

One day, this function might do more than just remove the CLT, so IMO
all this function body should be within a check:

if (OidIsValid(subconflictlogrelid))
{
/* Drop any dependent CLT */
...
}

~~~

DropSubscription

6.
+ if (OidIsValid(subconflictlogrelid))
+ drop_subscription_dependencies(subid, subname, subconflictlogrelid);

Make it unconditional. Instead, add the condition inside the
'drop_subscription_dependencies', per the previous review comment #5.

======
src/test/regress/sql/subscription.sql

7.
+--
+-- PUBLICATION: Verify conflict log tables are not publishable
+--
+-- pg_relation_is_publishable should return false for internal conflict log
+-- tables to prevent them from being accidentally included in publications
+--

Everywhere else, you had removed the word "internal", but this one
(maybe others?) was missed.

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