Re: Proposal: Conflict log history table for Logical Replication

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

In response to

Browse pgsql-hackers by date

  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