Re: Proposal: Conflict log history table for Logical Replication

On Sat, Jan 10, 2026 at 12:19 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, Jan 9, 2026 at 8:30 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi Dilip.
> >
> > Here are some review comments for the v19-0002 (code only, not tests).
> >
> > ======
> > src/backend/replication/logical/conflict.c
> >
> > 1.
> > +/* Schema for the elements within the 'local_conflicts' JSON array */
> > +static const ConflictLogColumnDef LocalConflictSchema[] =
> > +{
> > + { .attname = "xid",       .atttypid = XIDOID },
> > + { .attname = "commit_ts", .atttypid = TIMESTAMPTZOID },
> > + { .attname = "origin",    .atttypid = TEXTOID },
> > + { .attname = "key",       .atttypid = JSONOID },
> > + { .attname = "tuple",     .atttypid = JSONOID }
> > +};
> >
> > Maybe this only needs to be used in this module, but I still thought
> > LocalConflictSchema[] (and the MAX_LOCAL_CONFLICT_INFO_ATTRS) belongs
> > more naturally alongside the other ConflictLogSchema[] (e.g. in
> > conflict.h)
>
> Since this is used only in conflict.c, my personal preference to keep
> it in the .c file
>
> > ~~~
> >
> > 2.
> > +
> > +#define MAX_LOCAL_CONFLICT_INFO_ATTRS \
> > + (sizeof(LocalConflictSchema) / sizeof(LocalConflictSchema[0]))
> > +
> >
> > how about:
> >
> > #define MAX_LOCAL_CONFLICT_INFO_ATTRS lengthof(LocalConflictSchema)
>
> Make sense
> > ~~~
> >
> > ReportApplyConflict:
> >
> > 3.
> > + /* Insert to table if destination is 'table' or 'all' */
> > + if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
> >
> > I think it is unnecessary to even mention 'all'. The bitmasks tell
> > everything you need to know.
> >
> > SUGGESTION
> > Insert to table if requested.
>
> Done
>
> > ~
> >
> > 4.
> > There's a slightly convoluted if;if/else with these bitmasks here, but
> > I think Shveta already suggested the same change as what I was
> > thinking.
>
> So now we have this, which is first handling the table case and
> handling the LOG case separately, because irrespective of whether we
> want to insert into the table or not we need to check what we need to
> log?  What's your suggestion for this?
>
> if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
> {
>  <Insert into table>
> }
>
> if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
> {
> <write full log>
> }
> else
> {
> <write minimal log>
> }
>
>
>
> > ~~~
> >
> > GetConflictLogTableInfo:
> >
> > 5.
> > + *log_dest = GetLogDestination(MySubscription->conflictlogdest);
> > + conflictlogrelid = MySubscription->conflictlogrelid;
> > +
> > + /* If destination is 'log' only, no table to open. */
> > + if (*log_dest == CONFLICT_LOG_DEST_LOG)
> > + return NULL;
> >
> > I think checking and mentioning 'log' here is not future-proof for
> > when there might be more kinds of destinations. We just want to return
> > when the user doesn't want a 'table'. Use the bitmasks to do that.
> >
> > SUGGESTION
> > /* Quick exit if a conflict log table was not requested. */
> > if ((*logdest & CONFLICT_LOG_DEST_TABLE) == 0)
> >   return NULL;
>
> Make sense
>
> > ~~~
> >
> > build_conflict_tupledesc:
> >
> > 6.
> > +static TupleDesc
> > +build_conflict_tupledesc(void)
> > +{
> >
> > Missing function comment. There used to be one (??).
>
> Yeah it got lost in some of the refactoring.
>
> > ~~~
> >
> > build_local_conflicts_json_array:
> >
> > 7.
> > + json_datum_array = (Datum *) palloc(num_conflicts * sizeof(Datum));
> > + json_null_array = (bool *) palloc0(num_conflicts * sizeof(bool));
> >
> > I may have already asked this same question in a previous review, but
> > shouldn't these be using those new palloc_array and palloc0_array
> > macros?
>
> Done.
>
> > ======
> > src/include/replication/conflict.h
> >
> > 8.
> > -/* Define the count using the array size */
> >  #define MAX_CONFLICT_ATTR_NUM (sizeof(ConflictLogSchema) /
> > sizeof(ConflictLogSchema[0]))
> >
> > This change appears to be in the wrong patch. AFAICT it belonged in 0001.
>
> Fixed.
>
> Will shared the updated patch after fixing other comments from Shveta
> as well, thanks for the review.
>

Here is the updated patch

1. fixes all the agreed comments from Peter and Shveta.

2. also for dependency between the clt and subscription I have
improvised the comments, I mean now we don't need that dependency to
protect the clt from getting dropped as that is taken care by internal
schema, we still maintain the dependency considering the same thing is
done for the toast table and these tables are internally
created/dropped along with subscription so conceptually it makes sense
to maintain the internal dependency and its also looks fine to drop it
explicitly using dependency dependency drop function i.e.
performDeletion() as we are doing similar things in other cases like
"Identity Columns".

3. I tried to merge the test cases in the existing 035_conflict.pl, I
modified the create subscription to log into both table and log and
then extended some of the existing test cases to validate the table
logging as well.  If we think this looks fine then we can update a few
more test cases to validate from the table or maybe all the test
cases?

--
Regards,
Dilip Kumar
Google