Re: Proposal: Conflict log history table for Logical Replication

On Thu, Apr 16, 2026 at 9:24 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, Apr 10, 2026 at 4:54 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > Hi Dilip,
> > I’m planning to review/test the feature patches, but they don’t apply
> > cleanly on the latest HEAD. Could you please rebase them?
> >
>  Thanks Nisha. Here is the rebased version

Thank you Dilip. I reviewed the patches and did basic testing.

Here are a few comments for the first two patches -
Patch-0001
-------
1) The new column subconflictlogdest in pg_subscription is currently nullable.
e.g.:
 set allow_system_table_mods = on;
 update pg_subscription set subconflictlogdest = NULL where oid=24580;
This leads to the apply worker failing with:

ERROR: unexpected null value in cached tuple for catalog
pg_subscription column subconflictlogdest
LOG: background worker "logical replication apply worker" exited with
exit code 1

This seems to be because the column is not defined as NOT NULL, while
GetSubscription() retrieves it using SysCacheGetAttrNotNull.
Since this column is expected to always have a valid value (log,
table, or all), it might be better to define it as NOT NULL, similar
to other such columns :

text subconflictlogdest BKI_FORCE_NOT_NULL;
~~~

2) Some of the header file inclusions appear to be unnecessary. I am
able to build without these newly added inclusions.
In src/backend/catalog/pg_publication.c
 #include "commands/publicationcmds.h"
+#include "commands/subscriptioncmds.h"

In src/backend/commands/subscriptioncmds.c
+#include "access/heapam.h"
+#include "commands/comment.h"
+#include "utils/regproc.h"
~~~

Patch-0002
-------
3)
+++ b/src/include/replication/conflict.h
+extern bool ValidateConflictLogTable(Relation rel);

The function ValidateConflictLogTable() doesn’t seem to be
implemented. Was it intended to be removed?

4) In conflic.c:build_local_conflicts_json_array(),
 -- the json_null_array allocated and freed without ever being used.
Looks like an oversight?

--
Thanks,
Nisha