Re: Proposal: Conflict log history table for Logical Replication

A few comments on v36-007:

1)

AlterSubscriptionConflictLogDestination
+ want_table = (logdest == CONFLICT_LOG_DEST_TABLE ||
+ logdest == CONFLICT_LOG_DEST_ALL);
+ 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?

2)
I think we can move 'AlterSubscriptionConflictLogDestination' into the
configuration patch itself (if needed). It is not directly used
anywhere in upgrade flow as such. IIUC, even if upgrade flow uses it,
it will only be used through AlterSubscription.

3)
AlterSubscriptionConflictLogDestination:

+ if (want_table && !has_oldtable)
+ {
+ char relname[NAMEDATALEN];
+
+ snprintf(relname, NAMEDATALEN, "pg_conflict_log_for_subid_%u", sub->oid);
+
+ /*
+ * In upgrade scenarios, the conflict log table already exists. Update
+ * the catalog to record the association.
+ */
+ relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE);
+ if (!OidIsValid(relid))
+ relid = create_conflict_log_table(sub->oid, sub->name, sub->owner);

So this function will now be used during upgrade where destination is
TABLE/ALL as well as regular Alter-Subscription to change destination
from LOG to TABLE/ALL. In upgrade case, we expect the relid (CLT) to
be present already while in regular case, we don't expect any CLT to
be present.

The above code does not take care of maintaining the sanity checks. It
should be able to distinguish the 2 cases and Assert/Error if the
condition is opposed to what we expect.

4)
Also , I do not understand how can upgrade ever pass this check:

+ if (want_table && !has_oldtable)

It is not obvious how the upgrade flow will pass this check because
theoretically both the old and new setup should have the exact same
configuration; i.e. if 'want_table' is true, 'has_oldtable' will be
true. We can add a comment to clarify the situation here.

thanks
Shveta