Re: Proposal: Conflict log history table for Logical Replication

On Wed, Jun 24, 2026 at 12:53 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Jun 23, 2026 at 2:22 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Few comments:
> > 1) Currently we are storing these in shared memory. Looking at the
> > implementation, these fields are purely worker-private state used to
> > ferry data across the error boundary from prepare_conflict_log_tuple()
> > (inside the PG_TRY block) to ProcessPendingConflictLogTuple() (inside
> > the PG_CATCH block).

Good point.

If it is not required by another process, should
> > it be moved out of shared memory.
> > +       /* A conflict log tuple that is prepared but not yet inserted. */
> > +       HeapTuple       conflict_log_tuple;
> > +
> > +       /*
> > +        * Error-context string describing the conflict above, used to
> > annotate any
> > +        * error raised while inserting conflict_log_tuple into the conflict log
> > +        * table.  Allocated, like conflict_log_tuple, in ApplyContext.
> > +        */
> > +       char       *conflict_log_errcontext;
>
> Yeah there is no need for them to be in shared memory, but do we have
> any other data sturcture where these fits naturally, or we can make
> them global variables?
>

Or we can have a file local struct PendingConflictLogData similar to
FlushPosition. See the attached top-up patch. As the comment
("Allocated, like conflict_log_tuple, in ApplyContext") says it is
allocated in process-local Apply context, it is not safe to keep them
in shared memory.

>
> > 4) Is the condition remote_commit_ts > 0 done intentionally?
> > +       if (remote_commit_ts > 0)
> > +               values[attno++] = TimestampTzGetDatum(remote_commit_ts);
> > +       else
> > +               nulls[attno++] = true;
> >
> > As I had seen some negative values for certain timestamps. Shouldn't
> > the check be != 0?
> > SELECT extract(epoch FROM '1969-12-31 23:59:59+00'::timestamptz);
> >   extract
> > -----------
> >  -1.000000
> > (1 row)
>
> I think the 0 can also be generated for timestamptz, but since we are
> initializing `timestamptz` with 0, checking it seems correct.= 0, but
> I need to put more thought into this.
>

I think either way (>0 or !=0) are fine as both will serve the desired
purpose but I think !=0 will be more robust because we are using 0 as
sentinel value for remote_commit_ts.

-- 
With Regards,
Amit Kapila.