Re: Proposal: Conflict log history table for Logical Replication

On Thu, Aug 13, 2026 at 3:22 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Aug 13, 2026 at 11:10 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Wed, Aug 12, 2026 at 9:13 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > >
> > > Changed.
> > >
> >
> > Thanks Dilip. A few trivial comments on v69:
> >
> > 1)
> > +insert_conflict_log_tuple(Relation conflictlogrel, HeapTuple tuple)
> > +{
> > + Assert(tuple != NULL);
> > +
> > + heap_insert(conflictlogrel, tuple,
> > + GetCurrentCommandId(true), 0, NULL);
> > +}
> >
> > Should we get rid of this function now?
> > a) It does not do anything extra other than heap_insert()
> > b) There is only one caller.
>
> So I have merged prepare_conflict_log_tuple() code into the
> insert_conflict_log_tuple() and kept the name as
> insert_conflict_log_tuple()
>
> > 2)
> > LOG: conflict detected on relation "public.tab1": conflict=delete_missing
> > DETAIL: Conflict details are logged to the conflict log table:
> > pg_conflict_log_16390
> >
> > It will be good to specify schema qualified name here:
> > pg_conflict.pg_conflict_log_16390, as we do at other places.
>
> Done
>
> >
> > 3)
> > The doc page shows:
> >
> > 29.8.1. Conflict logging
> > 29.8.2. Table-based logging
> > 29.8.3. File-based logging
> > 29.8.4. Notes
> >
> > Should it be:
> > 29.8.1. Conflict logging
> > 29.8.1.1. Table-based logging
> > 29.8.1.2. File-based logging
> > 29.8.2. Notes
> >
> > What do you say?
>
> Yeah I have moved 29.8.1.1, Table-based logging and 29.8.1.2,
> File-based logging section into the 29.8.1. Conflict logging section,
> but from the main page I have only kept the link unabled for the
> 29.8.1. Conflict logging this is same as how it is done for other
> subsections, e.g. 29.14.3.1 etc..
>
>

Thanks, a few trivial things:

1)
Currently in '29.8. Conflicts', we have:
29.8.1.1. Table-based logging
29.8.1.2. File-based logging

I feel '29.8.1.2. File-based logging' should come first (i.e. as
29.8.1.1) followed by 'Table-based' logging. Reasons:
a) default should be explained first.
b) Table-based logging refers to File-based logging, so the latter
should be explained first.
c) order in create-sub and 'Conflicts' page should be same.

2)
tuple_table_slot_to_indextup_json() has this:

+ Assert(CheckRelationOidLockedByMe(indexid, RowExclusiveLock, true));

We shall add a comment similar to one atop build_index_value_desc():
* The caller must ensure that the index with the OID 'indexoid' is locked so
* that .....

3)
+/*
+ * build_conflict_tupledesc
+ *
+ * Build and bless a tuple descriptor for the conflict log table based on the
+ * predefined LocalConflictSchema.
+ */
+static TupleDesc
+build_conflict_tupledesc(void)

It is a generic name, perhaps we should rename to
build_local_conflicts_tupledesc, based on caller
build_local_conflicts_json_array.
(Do it if you agree, not manadatory).

4)
It will be good to have 'Assert(conflictlogrel != NULL)' inside
insert_conflict_log_tuple as well before we access conflictlogrel.

~~

I do not have any further comments. The patch is good otherwise.

thanks
Shveta