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

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.
1. I'm changing the ConflictLogDest enum from bitmap to integer. I can
revert this in the next version but I want to see Peter's opinion
first, as he suggested using a bitmap to easily apply bitwise
operators.

2. Change how to display conflict log table in \dRs+ as suggested by
Shveta and Amit have agreement on the same, I will update that in next
version.

3. As Vignesh reported, we are still determining the best way to
change the client's ownership when the subscription ownership changes.

4. pg_conflict is the catalog schema and as Nisha reported,
non-superusers aren't allowed to access the objects within it. Because
of this, SELECT, DELETE, and TRUNCATE are disallowed even for the
subscription owner if that owner is a non-superuser. I am working on
the fix.


Note: I have included the base patch for reporting the schema
qualified name, which is also being discussed in other thread,
@vignesh C you need to rebase your patch and might need to fix the
table name, as we are now using `pg_conflict_log_<subid>` for the
conflict log table.

--
Regards,
Dilip Kumar
Google