Re: Fix replica identity checks for MERGE command on published table.

On Thu, 21 Aug 2025 at 04:41, Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Friday, July 11, 2025 3:09 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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.

Regards,
Dean