Re: Proposal: Conflict log history table for Logical Replication

On Wed, Jun 24, 2026 at 5:54 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Wed, 24 Jun 2026 at 16:32, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Jun 24, 2026 at 12:53 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Jun 23, 2026 at 2:22 PM vignesh C <vignesh21(at)gmail(dot)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