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
