Обсуждение: Fix replica identity checks for MERGE command on published table.

Hi,

While testing publication DDLs, I noticed an unexpected behavior where the
MERGE command can be executed on tables lacking replica identity keys,
regardless of whether they are part of a publication that publishes updates and
deletes.

Replication and application of the updates and deletes generated by MERGE
command require replica identity keys in the WAL record, which are essential
for the apply worker on the subscriber to find local tuples for updating or
deletion. Furthermore, publications require specific columns to be part of the
replica identity key if the table specifies a publication row filter or column
list.

We already have restrictions on executing UPDATE and DELETE commands for tables
without replica identity keys under similar conditions. So, I think the same
restriction should apply to the MERGE command as well.

Attached is a patch that implements this fix.

I have confirmed that this issue has existed since the introduction of the
MERGE command in PG15.

Best Regards,
Hou zj

On Fri, Apr 11, 2025 at 10:54 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Hi,
>
> While testing publication DDLs, I noticed an unexpected behavior where the
> MERGE command can be executed on tables lacking replica identity keys,
> regardless of whether they are part of a publication that publishes updates and
> deletes.
>
> Replication and application of the updates and deletes generated by MERGE
> command require replica identity keys in the WAL record, which are essential
> for the apply worker on the subscriber to find local tuples for updating or
> deletion. Furthermore, publications require specific columns to be part of the
> replica identity key if the table specifies a publication row filter or column
> list.
>
> We already have restrictions on executing UPDATE and DELETE commands for tables
> without replica identity keys under similar conditions. So, I think the same
> restriction should apply to the MERGE command as well.
>

+-- fail - missing REPLICA IDENTITY
+MERGE INTO testpub_merge_no_ri USING testpub_merge_pk s ON s.a >= 1
+ WHEN MATCHED THEN UPDATE SET b = s.b;
+ERROR:  cannot update table "testpub_merge_no_ri" because it does not
have a replica identity and publishes updates
+HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
+-- fail - missing REPLICA IDENTITY
+MERGE INTO testpub_merge_no_ri USING testpub_merge_pk s ON s.a >= 1
+ WHEN MATCHED THEN DELETE;
+ERROR:  cannot delete from table "testpub_merge_no_ri" because it
does not have a replica identity and publishes deletes
+HINT:  To enable deleting from the table, set REPLICA IDENTITY using
ALTER TABLE.

I was wondering whether we should mention MERGE somewhere in the error
message like "cannot merge into table ...". But the error message is
reported depending upon the actual operation being performed and
whether it's being published by the publication, so mentioning
specific operations is better than mentioning just MERGE. So I think
the current error message is ok; and it will help point out the
operations that caused it.

But that opens up another question: some merge operations (on the same
table) will go through and some won't if the publication publishes
only some of the operations. I am wondering, albeit quite late after
the feature has sailed, whether MERGE should be considered a separate
operation as far as publication is concerned. This topic may have been
discussed either when MERGE was implemented or when operation filters
were implemented. Sorry for the noise in that case.

This isn't a detailed review.

--
Best Wishes,
Ashutosh Bapat

On Mon, 14 Apr 2025 at 05:40, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> I was wondering whether we should mention MERGE somewhere in the error
> message like "cannot merge into table ...". But the error message is
> reported depending upon the actual operation being performed and
> whether it's being published by the publication, so mentioning
> specific operations is better than mentioning just MERGE. So I think
> the current error message is ok; and it will help point out the
> operations that caused it.

Yes, agreed. The error messages seem clear enough to me.

> But that opens up another question: some merge operations (on the same
> table) will go through and some won't if the publication publishes
> only some of the operations. I am wondering, albeit quite late after
> the feature has sailed, whether MERGE should be considered a separate
> operation as far as publication is concerned. This topic may have been
> discussed either when MERGE was implemented or when operation filters
> were implemented. Sorry for the noise in that case.

I don't know if it was discussed at the time, but to me the way it has
been done makes sense, because replication is happening at a lower
level -- individual row INSERTs, UPDATEs and DELETEs using rows
identified by the replica identity key. If you tried to do it at the
level of MERGE, I think it would be difficult to guarantee the same
results on the replica.

This makes me wonder though, does INSERT ... ON CONFLICT DO UPDATE
have the same problem as MERGE?

If I create a table and publication like this:

CREATE TABLE foo (a int PRIMARY KEY, b text, c int);
INSERT INTO foo VALUES (1, 'xxx', 10);

CREATE PUBLICATION pub1 FOR TABLE foo WHERE (c > 0);

plain UPDATEs are rejected:

UPDATE foo SET b = 'yyy' WHERE a = 1;

ERROR:  cannot update table "foo"
DETAIL:  Column used in the publication WHERE expression is not part
of the replica identity.

However, INSERT ... ON CONFLICT DO UPDATE can be used to achieve the same thing:

INSERT INTO foo VALUES (1)
  ON CONFLICT (a) DO UPDATE SET b = 'yyy';

This isn't rejected, and it replicates the change to the subscriber
correctly, which makes me wonder, why do we need this restriction on
filter columns? If there is some other situation where it's needed,
should INSERT ... ON CONFLICT DO UPDATE also check it?

Regards,
Dean

On Mon, 7 Jul 2025 at 18:17, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> This makes me wonder though, does INSERT ... ON CONFLICT DO UPDATE
> have the same problem as MERGE?
>

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,
for example:

CREATE TABLE foo (a int, b text);
INSERT INTO foo VALUES (1, 'xxx');
CREATE PUBLICATION pub1 FOR TABLE foo;
CREATE UNIQUE INDEX foo_a_idx ON foo(a);

Then a plain UPDATE is blocked:

UPDATE foo SET b = 'yyy' WHERE a = 1;

ERROR:  cannot update table "foo" because it does not have a replica
identity and publishes updates
HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.

but the equivalent with INSERT ... ON CONFLICT DO UPDATE is allowed on
the publisher:

INSERT INTO foo VALUES (1)
  ON CONFLICT (a) DO UPDATE SET b = 'yyy';

but fails on the subscriber:

ERROR:  logical replication target relation "public.foo" has neither
REPLICA IDENTITY index nor PRIMARY KEY and published relation does not
have REPLICA IDENTITY FULL
CONTEXT:  processing remote data for replication origin "pg_16535"
during message type "UPDATE" for replication target relation
"public.foo" in transaction 872, finished at 0/01E65718

So INSERT ... ON CONFLICT DO UPDATE needs to check that the result
relation is valid for UPDATEs.

Regards,
Dean

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.

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.

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. 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.

Regards,
Dean

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

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.

Regards,
Dean

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

On Thu, 28 Aug 2025 at 07:17, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> 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).

Thanks.

I've pushed these, though I swapped the order of commit, and of the
arguments for CheckValidResultRel() because INSERT ON CONFLICT DO
UPDATE is older than MERGE, so it seemed more natural to put that
first.

Regards,
Dean