On Mon, Jan 19, 2026 at 7:57 AM Peter Smith <[email protected]> wrote:
>
> Some review comments for v22-0001
>
> ======
> src/backend/executor/execMain.c
>
> 1.
> + /*
> + * Conflict logging tables (CLT) are managed by the system to record
> + * replication conflicts. We allow DELETE to permit users to manually prune
> + * or truncate these logs, but manual data insertion or modification
> + * (INSERT, UPDATE, MERGE) is prohibited to maintain the integrity of the
> + * system-generated logs.
> + */
> + if (IsConflictNamespace(RelationGetNamespace(resultRel)) &&
> + operation != CMD_DELETE)
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("cannot execute %s on conflict logging table \"%s\"",
> + (operation == CMD_INSERT ? "INSERT" :
> + operation == CMD_UPDATE ? "UPDATE" :
> + operation == CMD_MERGE ? "MERGE" : "this operation"),
> + RelationGetRelationName(resultRel)),
> + errdetail("Conflict logging tables are system-managed and only
> support cleanup via DELETE or TRUNCATE.")));
>
> 1a.
> For consistency with every other place, "Conflict logging table"
> should be changed to "Conflict log table". (See comment, errmsg,
> errdetail).
>
> ~
>
> 1b.
> Naming the failed operation seemed unnecessary. Also, the way it is
> currently implemented, having "this operation" substitution looks like
> bad practice for translations. Consider just a simpler message.
>
> SUGGESTION
> cannot modify or insert data for conflict log table \"%s\"
Make sense.
> ======
> src/bin/psql/describe.c
>
> 2.
> + appendPQExpBuffer(&buf,
> + ", (CASE WHEN subconflictlogdest IN ('table', 'all') "
> + " THEN 'pg_conflict.pg_conflict_' || oid "
> + " ELSE '-' END) AS \"%s\"\n",
> + gettext_noop("Conflict log table"));
>
> I'm not sure if you really needed to include the 'pg_conflict' schema
> name here. It's just taking up space, and it's already well-defined in
> the documentation that CLT only live in the 'pg_conflict' schema.
Right, we can remove the schema name from here.
--
Regards,
Dilip Kumar
Google
