Re: Proposal: Conflict log history table for Logical Replication

On Wed, 24 Jun 2026 at 19:27, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> 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

Thanks for the updated patch, few comments for v56-0001 patch:
1) In namespace.c, the comment has been updated to state that users
cannot move tables into the pg_conflict namespace. However, I don't
see any corresponding code here that checks whether the target
namespace is pg_conflict. Should this comment be removed, or should it
instead explain how moves into the conflict schema are prevented
elsewhere?
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -3523,9 +3523,8 @@ LookupCreationNamespace(const char *nspname)
/*
* Common checks on switching namespaces.
*
- * We complain if either the old or new namespaces is a temporary schema
- * (or temporary toast schema), or if either the old or new namespaces is the
- * TOAST schema.
+ * We complain if either the old or new namespaces is a temporary schema,
+ * temporary toast schema, the TOAST schema, or the conflict schema.
*/
void
CheckSetNamespace(Oid oldNspOid, Oid nspOid)

2) jsonb supports indexing, whereas json does not. Was json chosen
here because inserts are faster due to the lack of parsing and binary
conversion? If so, it would be helpful to add a brief comment
explaining why json was chosen over jsonb, so that the rationale is
clear to future readers:
+static const ConflictLogColumnDef ConflictLogSchema[] = {
+ { .attname = "relid", .atttypid = OIDOID },
+ { .attname = "schemaname", .atttypid = TEXTOID },
+ { .attname = "relname", .atttypid = TEXTOID },
+ { .attname = "conflict_type", .atttypid = TEXTOID },
+ { .attname = "remote_xid", .atttypid = XIDOID },
+ { .attname = "remote_commit_lsn",.atttypid = LSNOID },
+ { .attname = "remote_commit_ts", .atttypid = TIMESTAMPTZOID },
+ { .attname = "remote_origin", .atttypid = TEXTOID },
+ { .attname = "remote_tuple", .atttypid = JSONOID },
+ { .attname = "replica_identity", .atttypid = JSONOID },

3) patch needs rebase, there is conflict with recent commit:
git am v56-0001-Add-configurable-conflict-log-table-for-Logical-.patch
Applying: Add configurable conflict log table for Logical Replication
error: patch failed: src/bin/psql/tab-complete.in.c:2376
error: src/bin/psql/tab-complete.in.c: patch does not apply

few typos:
4) Here "an conflict log table" should be "a conflict log table"
+-- this should generate an conflict log table named pg_conflict_log_$subid$
+CREATE SUBSCRIPTION regress_conflict_test1 CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect =
false, conflict_log_destination = 'table');

5) Here "conflict logs tables" should be "conflict log tables"
+-- This should fail as the namespace is reserved for conflict logs tables
+CREATE TABLE pg_conflict.manual_table (id int);

Regards,
Vignesh