Thread
-
Re: Proposal: Conflict log history table for Logical Replication
vignesh C <vignesh21@gmail.com> — 2026-05-06T03:54:01Z
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