Re: Proposal: Conflict log history table for Logical Replication

Hi Dilip/Vignesh.

Some review comments for v33-0001.

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

pg_class_aclmask_ext:

1.
if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE |
ACL_USAGE)) &&
- IsSystemClass(table_oid, classForm) &&
- classForm->relkind != RELKIND_VIEW &&
+ IsConflictClass(classForm) &&
!superuser_arg(roleid))
- mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE);
+ mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_USAGE);
+ else if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE |
ACL_TRUNCATE | ACL_USAGE)) &&
+ IsSystemClass(table_oid, classForm) &&
+ classForm->relkind != RELKIND_VIEW &&
+ !superuser_arg(roleid))
+ mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE);

The new patched code seems a bit repetitive.

How about refactoring like below and putting the comments where they belong.

if (!superuser_arg(roleid))
{
if (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE))
{
if (IsSystemClass(table_oid, classForm) &&
classForm->relkind != RELKIND_VIEW)
{
/*
* Deny anyone permission to update a system catalog unless
* pg_authid.rolsuper is set.
*
* As of 7.4 we have some updatable system views; those shouldn't be
* protected in this way. Assume the view rules can take care of
* themselves. ACL_USAGE is if we ever have system sequences.
*/
mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE |
ACL_USAGE);
}
else if (IsConflictClass(classForm))
{
/*
* For conflict log tables, we allow non-superusers to perform DELETE
* and TRUNCATE for maintenance, while still restricting INSERT,
* UPDATE, and USAGE.
*/
mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_USAGE);
}
}
}
else
{
/* Superusers bypass all permission-checking. */

ReleaseSysCache(tuple);
return mask;
}

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

IsConflictClass:

2.
+/*
+ * IsConflictClass - Check if the given pg_class tuple belongs to the conflict
+ * namespace.
+ */

This function comment looks different from all the nearby ones where
the function name appears on a line by itself.

======
src/backend/catalog/heap.c

heap_create:

3.
if (!allow_system_table_mods &&
((IsCatalogNamespace(relnamespace) && relkind != RELKIND_INDEX) ||
- IsToastNamespace(relnamespace)) &&
+ IsToastNamespace(relnamespace) ||
+ IsConflictNamespace(relnamespace)) &&

Is this code correct? It seems like it is conveniently re-using a
similar error, which is not quite appropriate.

The comment refers to creating relations in pg_catalog.
The errdetail refers to "System catalog modifications"

But, the CLT is neither in pg_catalog schema, nor is it a system catalog.

======
src/backend/catalog/namespace.c

CheckSetNamespace:

4.
- * We complain if either the old or new namespaces is a temporary schema
- * (or temporary toast schema), or if either the old or new namespaces is the
- * TOAST schema.
+ * We complain if either the old or new namespaces is a temporary schema,
+ * temporary toast schema, the TOAST schema, or the CONFLICT schema.

TOAST is uppercase because it is an acronym, but I see no reason why
"CONFLICT" is uppercase. Maybe replace that with pg_conflict.

~~~

5.
+
+ /* similarly for CONFLICT schema */
+ if (nspOid == PG_CONFLICT_NAMESPACE || oldNspOid == PG_CONFLICT_NAMESPACE)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move objects into or out of CONFLICT schema")));

Ditto for the uppercase "CONFLICT" in the comment and in the errmsg.
Say pg_conflict.

======
src/backend/catalog/pg_publication.c

6.
+
+ /* Can't be conflict log table */
+ if (IsConflictNamespace(RelationGetNamespace(targetrel)))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(errormsg, relname),
+ errdetail("This operation is not supported for conflict log tables.")));

I felt this code is quite similar to the "Can't be system table"
check, so it might be better to move it to be adjacent to that.

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

CreateSubscription:

7.
+ /* Always set the destination, default will be 'log'. */
+ values[Anum_pg_subscription_subconflictlogdest - 1] =
+ CStringGetTextDatum(ConflictLogDestNames[opts.conflictlogdest]);

None of the other values[] assignments here have comments talking
about defaults etc, so why is this one different?

~~~

8.
Despite some of these just being static, I am beginning to think that
the "conflict" specific CLT code might be more appropriate to be put
in conflict.c, along with the CLT schema etc.

e.g. functions like:
- create_conflict_log_table_tupdesc
- create_conflict_log_table
- GetLogDestination

~~~

create_conflict_log_table:

9.
+ snprintf(relname, NAMEDATALEN, "pg_conflict_log_%u", subid);

Would it be more helpful if the generated table name describes what
that %u means?

e.g. "pg_conflict_log_for_subid_%u"

~~~

10.
+ /*
+ * Check for an existing table with the sname name in the pg_conflict
namespace.
+ * A collision should not occur under normal operation, but we must
handle cases
+ * where a table has been created manually.
+ */
+ if (OidIsValid(get_relname_relid(relname, PG_CONFLICT_NAMESPACE)))
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_TABLE),
+ errmsg("conflict log table pg_conflict.\"%s\" already exists", relname),
+ errhint("A table with the same name already exists. "
+ "To proceed, drop the existing table and retry.")));
+

10a.
Typo /sname name/same name/

~

10b.
That 1st sentence of the errhint seems unnecessary because it is
saying the same as the errmsg.

======
src/backend/executor/execMain.c

11.
+
+ /*
+ * Conflict log tables are managed by the system to record logical
+ * replication conflicts. We allow DELETE and TRUNCATE to permit users to
+ * manually prune these logs, but manual data insertion or modification
+ * (INSERT, UPDATE, MERGE) is prohibited to maintain the integrity of the
+ * system-generated logs.
+ *
+ * Since TRUNCATE is handled as a separate utility command, we only need
+ * to explicitly permit CMD_DELETE here.
+ */
+ if (IsConflictNamespace(RelationGetNamespace(resultRel)) &&
+ operation != CMD_DELETE)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("cannot modify or insert data into conflict log table \"%s\"",
+ RelationGetRelationName(resultRel)),
+ errdetail("Conflict log tables are system-managed and only support
cleanup via DELETE or TRUNCATE.")));

It somehow feels backwards to check "operation != CMD_DELETE", with
the obscure comment that TRUNCATE is handled elsewhere.

How about just check if "(operation == CMD_INSERT || operation ==
CMD_UPDATE || operation == CMD_MERGE)".

~~~

12.
+
+ /*
+ * Conflict log tables are managed by the system to record logical
+ * replication conflicts. We do not allow locking rows in CONFLICT
+ * relations.
+ */
+ if (IsConflictNamespace(RelationGetNamespace(rel)))
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot lock rows in conflict log table \"%s\"",
+ RelationGetRelationName(rel))));

I was not sure what was meant by "CONFLICT relations.".

Does it mean "... relations in the pg_conflict schema.". Anyway, is
there any value to that 2nd sentence because it is much the same text
as the errmsg.

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

13.
+const char *const ConflictLogDestNames[] = {
+ [CONFLICT_LOG_DEST_LOG] = "log",
+ [CONFLICT_LOG_DEST_TABLE] = "table",
+ [CONFLICT_LOG_DEST_ALL] = "all"
+};
+
+const ConflictLogColumnDef v[] = {
+ { .attname = "relid", .atttypid = OIDOID },
+ { .attname = "schemaname", .atttypid = TEXTOID },
+ { .attname = "relname", .atttypid = TEXTOID },
+ { .attname = "conflict_type", .atttypid = TEXTOID },
+ { .attname = "remote_xid", .atttypid = XIDOID },
+ { .attname = "remote_commit_lsn",.atttypid = LSNOID },
+ { .attname = "remote_commit_ts", .atttypid = TIMESTAMPTZOID },
+ { .attname = "remote_origin", .atttypid = TEXTOID },
+ { .attname = "replica_identity", .atttypid = JSONOID },
+ { .attname = "remote_tuple", .atttypid = JSONOID },
+ { .attname = "local_conflicts", .atttypid = JSONARRAYOID }
+};

13a.
Both these arrays could benefit with some comments.

~

13b.
In the ConflictLogSchema, would it be better to keep all those
"remote_" columns grouped together, instead of being broken by
"replica_identity".

~

13c.
TBH, I preferred code how it used to be -- where all the CLT constants
and structs and enums and schemas were kept together. Now they are
split across conflict.h and conflict.c making it harder to read as
well as introducing need for static asserts that were not needed
before.

(Keeping everything together might become easier if the CLT stuff is
all colocated in the conflict.c per comment #8)

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