| From: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
|---|---|
| To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
| Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Proposal: Conflict log history table for Logical Replication |
| Date: | 2026-01-19 04:18:00 |
| Message-ID: | CAFiTN-vRVbfCvdV280GbVg79Nt=7yTi+jTiBBn0AMQNKej47Hg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon, Jan 19, 2026 at 7:57 AM Peter Smith <smithpb2250(at)gmail(dot)com> 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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | shveta malik | 2026-01-19 04:41:02 | Re: Simplify code building the LR conflict messages |
| Previous Message | jian he | 2026-01-19 04:13:20 | Re: ALTER DOMAIN ADD NOT NULL NOT VALID |