Re: Proposal: Conflict log history table for Logical Replication - Mailing list pgsql-hackers

On Fri, 1 May 2026 at 19:16, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Apr 30, 2026 at 10:40 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Wed, Apr 29, 2026 at 12:34 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Wed, Apr 29, 2026 at 11:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 28, 2026 at 7:53 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > > > > 2.
> > > > > > +typedef enum ConflictLogDest
> > > > > > +{
> > > > > > + /* Log conflicts to the server logs */
> > > > > > + CONFLICT_LOG_DEST_LOG   = 1 << 0,   /* 0x01 */
> > > > > > +
> > > > > > + /* Log conflicts to an internally managed conflict log table */
> > > > > > + CONFLICT_LOG_DEST_TABLE = 1 << 1,   /* 0x02 */
> > > > > > +
> > > > > > + /* Convenience bitmask for all supported destinations */
> > > > > > + CONFLICT_LOG_DEST_ALL   = (CONFLICT_LOG_DEST_LOG | CONFLICT_LOG_DEST_TABLE)
> > > > > > +} ConflictLogDest;
> > > > > > +
> > > > > > +/*
> > > > > > + * Array mapping for converting internal enum to string.
> > > > > > + */
> > > > > > +static const char *const ConflictLogDestNames[] = {
> > > > > > + [CONFLICT_LOG_DEST_LOG] = "log",
> > > > > > + [CONFLICT_LOG_DEST_TABLE] = "table",
> > > > > > + [CONFLICT_LOG_DEST_ALL] = "all"
> > > > > > +};
> > > > > >
> > > > > > Defining an array this way could be an Array size issue. Actually the
> > > > > > array has just three elements so the last element should be at
> > > > > > ConflictLogDestNames[2] but if we go by the above definition, it will
> > > > > > be ConflictLogDestNames[3]. Can we define by referring the following
> > > > > > existing way:
> > > >
> > > > I was analyzing this because I remember we were initially using the
> > > > format you suggested and switched to the bit format to enable direct
> > > > bitwise operations elsewhere.  I think Peter suggested that [1], and
> > > > the argument was that the bitwise operation is easy if we represent
> > > > them as a bit. Also, since we would not have too many options, the
> > > > array size shouldn't be an issue.  But I understand your point: adding
> > > > more elements will cause the array size to grow very fast as this is
> > > > using sparse array.  Let's see what others think about this, and then
> > > > we can decide whether to change it back?
> > > >
> > >
> > > The benefit of the current approach is that checking whether the
> > > destination is TABLE becomes straightforward:
> > >
> > > IsSet(opts.conflictlogdest,CONFLICT_LOG_DEST_TABLE)
> > >
> > > if we go by regular enum values (simialr to XLogSource), then it will be:
> > >
> > >  if (opts.logdest == CONFLICT_LOG_DEST_TABLE ||
> > >      opts.logdest == CONFLICT_LOG_DEST_ALL)
> >
> > Right
> >
> > > For ease of extending the enum and its corresponding text mappings, my
> > > personal preference is still the regular (non-bitwise) enum approach.
> >
> > Yeah, that's my personal preference too.  But Peter had strong stand
> > on keeping as bitwise so that we can directly use
> > IsSet(opts.conflictLogDest, CONFLICT_LOG_DEST_TABLE) operations.
> > Since this array shouldn't have many options, a sparse array is not an
> > issue.  So lets see what @Peter Smith has to say here and then we can
> > build a concensus on this.
> >
> > > But if we anticipate adding more destination options in the future
> > > that would be covered by ALL, checking for those in code could lead to
> > > growing chains of OR conditions, whereas the bitwise approach scales
> > > more cleanly in that respect. So I think the choice depends on what
> > > kinds of future extensions we expect.
> > >
> > > Do we have plans to add more options that would naturally fall under
> > > ALL? Or do we instead expect additions that are mutually exclusive;
> > > for example, splitting CONFLICT_LOG_DEST_LOG into something like
> > > CONFLICT_LOG_DEST_JSON_LOG and CONFLICT_LOG_DEST_TEXT_LOG, which may
> > > not make sense to group under ALL in the same way?
> >
> > Currently, I haven't considered which options would naturally fall
> > under "ALL." Perhaps if we plan targets other than logs and files,
> > those might also fall under "ALL."
>
> I have fixed all the reported comments except these four.

Few comments:
1) Currently we allow renaming of pg_conflict schema, this might be ok
as we allow other sysem schema like pg_catalog and pg_toast also.
postgres=# alter schema pg_conflict rename to test_conflict;
ALTER SCHEMA

While displaying the conflict table we will have to display the
renamed schema name instead of hard coding the schema name:
postgres=# \dRs+

List of subscriptions
 Name |  Owner  | Enabled | Publication | Binary | Streaming |
Two-phase commit | Disable on error | Origin | Password required | Run
as owner? | Failover | Server | Retain dead tuples | Max
 retention duration | Retention active | Synchronous commit |
       Conninfo                 | Receiver timeout |  Skip LSN  |
Description | Conflict log destination |        Confl
ict log table

------+---------+---------+-------------+--------+-----------+------------------+------------------+--------+-------------------+---------------+----------+--------+--------------------+----

--------------------+------------------+--------------------+------------------------------------------+------------------+------------+-------------+--------------------------+-------------
----------------------
 sub1 | vignesh | t       | {pub1,pub2} | f      | parallel  | d
         | f                | any    | t                 | f
  | f        |        | f                  |
                  0 | f                | off                |
dbname=postgres host=localhost port=5432 | -1               |
0/00000000 |             | table                    | pg_conflict.
pg_conflict_log_16397
(2 rows)

postgres=# select * from pg_conflict.pg_conflict_log_16397;
ERROR:  relation "pg_conflict.pg_conflict_log_16397" does not exist
LINE 1: select * from pg_conflict.pg_conflict_log_16397;

+               /* Conflict log destination is supported in v19 and higher */
+               if (pset.sversion >= 190000)
+               {
+                       appendPQExpBuffer(&buf,
+                                                         ",
subconflictlogdest AS \"%s\"\n",
+
gettext_noop("Conflict log destination"));
+
+                       appendPQExpBuffer(&buf,
+                                                         ", (CASE
WHEN subconflictlogdest IN ('table', 'all') "
+                                                         " THEN
'pg_conflict.pg_conflict_log_' || oid "
+                                                         " ELSE '-'
END) AS \"%s\"\n",
+
gettext_noop("Conflict log table"));
+               }

2) We will have to use the renamed schema here instead of hard coding:
+       /*
+        * 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.")));

3) Similarly here too:
+       /* Release tuple descriptor memory. */
+       FreeTupleDesc(tupdesc);
+
+       ereport(NOTICE,
+                       (errmsg("created conflict log table
pg_conflict.\"%s\" for subscription \"%s\"",
+                                       relname, subname)));

Regards,
Vignesh