On Wednesday, August 27, 2025 7:13 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> table.
>
> On Thu, 21 Aug 2025 at 04:41, Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> > On Friday, July 11, 2025 3:09 PM Dean Rasheed
> <dean.a.rasheed@gmail.com> wrote:
> > >
> > > In HEAD, it would be OK to change the signature of
> > > CheckValidResultRel() and pass it an onConflictAction argument to
> > > fix the ON CONFLICT DO UPDATE issue. However, I don't think that
> > > such a change would be back-patchable.
> >
> > Yes, I agree that we cannot alter the function signature in the back branches.
> > An alternative approach could be to introduce an additional call to
> > CheckCmdReplicaIdentity in the back branches, although this would
> > result in code that is less consistent with the HEAD branch:
> >
> > @@ -4243,6 +4248,16 @@ ExecInitModifyTable(ModifyTable *node, EState
> *estate, int eflags)
> > */
> > CheckValidResultRel(resultRelInfo, operation);
> >
> > +
> > + if (node->onConflictAction == ONCONFLICT_UPDATE)
> > +
> > + CheckCmdReplicaIdentity(resultRelInfo->ri_RelationDesc,
> CMD_UPDATE);
> >
>
> I think a better approach is to introduce a new function
> CheckValidResultRelNew() with the extra arguments, rather than changing all
> the callers in this way (for example, see
> ExecBRDeleteTriggersNew() and similar functions in back-branches).
>
> > > Arguably though, the ON CONFLICT DO UPDATE issue is much less likely
> > > to be a problem in practice, since it looks like it only happens if
> > > the table definition is changed after creating the publication.
> >
> > Regarding the issue with ON CONFLICT DO UPDATE, I think it can arise
> > even without modifications to the table definition. This is because
> > tables lacking a replica identity can be added directly to a
> > publication; we do not check replica identity on publication DDLs. The
> > original design intends for these checks to occur dynamically, such as during
> the execution of commands.
>
> Ah, good point.
>
> > However, I'm not
> > adamant about back-patching this since we haven't received any complaints
> yet.
>
> Hmm, I'm not so sure. It looks to me as though this bug can break replication in
> a somewhat nasty way -- this kind of setup might successfully replicate plain
> INSERTs for a long time, then someone does a MERGE or INSERT ... ON
> CONFLICT DO UPDATE which appears to work, but silently breaks replication
> of all subsequent plain INSERTs.
> The user might not notice that anything is wrong on the replica for a long time,
> which is not good.
>
> Therefore, I think both fixes should be back-patched.
>
> > I think the fix for MERGE cannot be directly applied to PG15 as well
> > because the mergeActions parameter is not initially passed to
> > CheckValidResultRel. To backpatch this issue, a similar approach to
> > the one discussed above might be
> > needed:
> >
> > @@ -4243,6 +4248,16 @@ ExecInitModifyTable(ModifyTable *node, EState
> *estate, int eflags)
> > */
> > CheckValidResultRel(resultRelInfo, operation);
> >
> > + foreach(lc, mergeActions)
> > + {
> > + MergeAction *action = (MergeAction *) lfirst(l);
> > +
> CheckCmdReplicaIdentity(resultRelInfo->ri_RelationDesc,
> > +
> action->commandType);
> > + }
>
> Again, I think it's best to do this by adding CheckValidResultRelNew() to
> back-branches.
>
> > I've prepared patches to address the MERGE and INSERT ON CONFLICT DO
> > UPDATE for the HEAD branch as a reference.
>
> Thanks. Those look reasonable to me on a quick read-through.
Thanks for the review. Since we agreed to back-patch both fixes, I am attaching
The patches for all branches (added a new function as suggested in back branches).
Best Regards,
Hou zj
