Re: Proposal: Conflict log history table for Logical Replication

Some review comments for v59-0001/0002.

//////
v59-0001
//////

======
src/backend/replication/logical/conflict.c

1.
+ *
+ * The tuple/key columns (replica_identity, remote_tuple, local_conflicts) are
+ * typed json rather than jsonb on purpose: they hold an exact audit snapshot
+ * of the applied tuples and replica identity, and json preserves the verbatim
+ * representation whereas jsonb would normalize it. Indexing them (jsonb's main
+ * advantage) wouldn't help anyway, as the conflict log is looked up by its
+ * scalar columns(relid, conflict_type, commit timestamp) while these JSON
+ * columns are per-conflict payload to inspect, not search keys.
+ */

Use consistent case. This comment has a mixture of json and JSON.

~~~

2.
+ { .attname = "remote_commit_lsn",.atttypid = LSNOID },
+ { .attname = "remote_commit_ts", .atttypid = TIMESTAMPTZOID },
+ { .attname = "remote_origin",    .atttypid = TEXTOID },
+ { .attname = "replica_identity_full", .atttypid = BOOLOID },
+ { .attname = "replica_identity",      .atttypid = JSONOID },
+ { .attname = "remote_tuple",          .atttypid = JSONOID },

The replica identity attributes are splitting the "remote" attributes.
Isn't it better to keep all the remote attributes grouped together?

~~~

3.
+/*
+ * Convert the string representation of a conflict logging destination to its
+ * corresponding enum value.
+ */
+ConflictLogDest
+GetConflictLogDest(const char *dest)
+{
+ /* NULL defaults to LOG. */
+ if (dest == NULL || pg_strcasecmp(dest, "log") == 0)
+ return CONFLICT_LOG_DEST_LOG;

Instead of default/NULL always being equivalent of 'log', what are the
thoughts about introducing a new GUC like
`default_conflict_log_destination` to override that default?

e.g
* SUBCRIPTION option `conflict_log_destination` valid values can be
"default" (default), "log", "table", "all"
* option conflict_log_destination = "default" means use value from GUC
`default_conflict_log_destination` which can be "log" (default) or
"table" or "all".

Existing code could then be configured to make use CLTs without having
to touch the individual subscription DDLs.
It might be useful when there are many subscriptions, or when a DBA
wants to set a site-wide policy (e.g. always log to tables) without
individual subscription management.


//////
v59-0002
//////

======
src/backend/catalog/aclchk.c

1.
+ /*
+ * Disallow creation in the conflict schema for everyone, including
+ * superusers, unless in binary-upgrade mode.
+ */
+ if (!IsBinaryUpgrade && (mask & ACL_CREATE) &&
+ IsConflictLogTableNamespace(nsp_oid))
+ return mask & ~ACL_CREATE;
+

Is this code OK? It only checks ACL_CREATE but how do you know the
rest of the mask is sane?

How about:
return mask & ACL_ALL_RIGHTS_SCHEMA & ~ACL_CREATE;

======
src/backend/commands/tablecmds.c

2.
+ /*
+ * Conflict log tables are managed by the system for logical
+ * replication and should not be used as parent tables, as
+ * inheritance could interfere with the logging behavior.
+ */
+ if (IsConflictLogTableNamespace(relation->rd_rel->relnamespace))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: cannot inherit from conflict log table \"%s\"",
+ RelationGetRelationName(relation)),
+ errdetail("Conflict log tables are system-managed tables for logical
replication conflicts.")));

It is not really a lack of "privilege" preventing this -- it is just
disallowed for everybody. Would ERRCODE_WRONG_OBJECT_TYPE be a more
appropriate errcode?

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