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

On Friday, July 11, 2025 3:09 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> 
> On Tue, 8 Jul 2025 at 09:51, Dean Rasheed <dean.a.rasheed@gmail.com>
> wrote:
> >
> > Answering my own question, INSERT ... ON CONFLICT DO UPDATE does
> have
> > the same problem as MERGE. To reproduce the error, all you need to do
> > is create the unique index it needs *after* creating the publication
> 
> The other question is whether we want to back-patch these fixes.

Thank you for looking into this issue, and sorry for the late reply.

> 
> 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);

> 
> 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. However, I'm not
adamant about back-patching this since we haven't received any complaints yet.

> So perhaps it would be acceptable to apply and back-patch the original
> patch for MERGE to v15, and fix the ON CONFLICT DO UPDATE issue in
> HEAD only.

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);
+		}

I've prepared patches to address the MERGE and INSERT ON CONFLICT DO UPDATE for
the HEAD branch as a reference.

Best Regards,
Hou zj