Re: Proposal: Conflict log history table for Logical Replication

On Wed, Jun 24, 2026 at 5:54 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, 24 Jun 2026 at 16:32, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > 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.
>
> These approach looks good, couple of minor comments can be done while
> merging the patch:
> 1) Generally we keep the typedef name and struct name same:
> +typedef struct PendingConflictLogData
> +{
> +       HeapTuple       tuple;                  /* prepared,
> not-yet-inserted conflict tuple */
> +       char       *errcontext_str;     /* conflict description for
> error context */
> +} PendingConflictLog;
>
> 2) PendingConflictLog should be included in typedefs.list
>
Please find the updated patch set attached. I have merged Amit's patch
and addressed the improvements suggested by Vignesh. Patch 0002 now
includes new test cases to cover all the newly reported error
conditions, except

1) I haven't yet added the test to cover the lock skip check in
LockViewRecurse_walker[1]. Since we now allow locking on the conflict
log table (CLT), it makes sense to acquire the lock even when it is
accessed recursively via a view.

2) Additionally, I haven't covered the error reported in
ATSimplePermissions() yet. We need more thought on how a CLT
permission error could actually be triggered in this function, given
that a simple ALTER TABLE ADD COLUMN is already blocked by the check
in RangeVarCallbackForAlterRelation().

[1]
@@ -198,6 +210,13 @@ LockViewRecurse_walker(Node *node,
LockViewRecurse_context *context)
                                relkind != RELKIND_VIEW)
                                continue;

+                       /*
+                        * Conflict log tables are managed by the
system for logical
+                        * and should not be locked explicitly.
+                        */
+                       if
(IsConflictLogTableNamespace(get_rel_namespace(relid)))
+                               continue;
+


-- 
Regards,
Dilip Kumar
Google