On Thu, Apr 16, 2026 at 9:24 PM Dilip Kumar <[email protected]> wrote: > > On Fri, Apr 10, 2026 at 4:54 PM Nisha Moond <[email protected]> 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
