Fix replica identity check for INSERT ON CONFLICT DO UPDATE.

If an INSERT has an ON CONFLICT DO UPDATE clause, the executor must
check that the target relation supports UPDATE as well as INSERT. In
particular, it must check that the target relation has a REPLICA
IDENTITY if it publishes updates. Formerly, it was not doing this
check, which could lead to silently breaking replication.

Fix by adding such a check to CheckValidResultRel(), which requires
adding a new onConflictAction argument. In back-branches, preserve ABI
compatibility by introducing a wrapper function with the original
signature.

Author: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Reviewed-by: Dean Rasheed <dean.a.rasheed@gmail.com>
Tested-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/OS3PR01MB57180C87E43A679A730482DF94B62@OS3PR01MB5718.jpnprd01.prod.outlook.com
Backpatch-through: 13

diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index fbbbc09a97b17aaab735be61b46e3bd9129a3be9..12781963b4f95bcdf7800f72a725705ea7e223c8 100644 (file)
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -919,7 +919,7 @@ CopyFrom(CopyFromState cstate)
    ExecInitResultRelation(estate, resultRelInfo, 1);
 
    /* Verify the named relation is a valid target for INSERT */
-   CheckValidResultRel(resultRelInfo, CMD_INSERT, NIL);
+   CheckValidResultRel(resultRelInfo, CMD_INSERT, ONCONFLICT_NONE, NIL);
 
    ExecOpenIndices(resultRelInfo, false);
 

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index b8b9d2a85f76ca6cdf7601538cb6e58d294fb950..8f56d5e312ec2f02353a3ee211f5a28b0a10324b 100644 (file)
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1036,6 +1036,9 @@ InitPlan(QueryDesc *queryDesc, int eflags)
  * Generally the parser and/or planner should have noticed any such mistake
  * already, but let's make sure.
  *
+ * For INSERT ON CONFLICT, the result relation is required to support the
+ * onConflictAction, regardless of whether a conflict actually occurs.
+ *
  * For MERGE, mergeActions is the list of actions that may be performed.  The
  * result relation is required to support every action, regardless of whether
  * or not they are all executed.
@@ -1045,7 +1048,7 @@ InitPlan(QueryDesc *queryDesc, int eflags)
  */
 void
 CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation,
-                   List *mergeActions)
+                   OnConflictAction onConflictAction, List *mergeActions)
 {
    Relation    resultRel = resultRelInfo->ri_RelationDesc;
    FdwRoutine *fdwroutine;
@@ -1059,6 +1062,13 @@ CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation,
        case RELKIND_RELATION:
        case RELKIND_PARTITIONED_TABLE:
            CheckCmdReplicaIdentity(resultRel, operation);
+
+           /*
+            * For INSERT ON CONFLICT DO UPDATE, additionally check that the
+            * target relation supports UPDATE.
+            */
+           if (onConflictAction == ONCONFLICT_UPDATE)
+               CheckCmdReplicaIdentity(resultRel, CMD_UPDATE);
            break;
        case RELKIND_SEQUENCE:
            ereport(ERROR,

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 514eae1037dc30c6bb066fb9a9c5f31488ed9d93..1f2da072632e3d19a2cbf0c0280a2644f8b1fee4 100644 (file)
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -360,8 +360,12 @@ ExecFindPartition(ModifyTableState *mtstate,
                                               true, false);
                if (rri)
                {
+                   ModifyTable *node = (ModifyTable *) mtstate->ps.plan;
+
                    /* Verify this ResultRelInfo allows INSERTs */
-                   CheckValidResultRel(rri, CMD_INSERT, NIL);
+                   CheckValidResultRel(rri, CMD_INSERT,
+                                       node ? node->onConflictAction : ONCONFLICT_NONE,
+                                       NIL);
 
                    /*
                     * Initialize information needed to insert this and
@@ -527,7 +531,8 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
     * partition-key becomes a DELETE+INSERT operation, so this check is still
     * required when the operation is CMD_UPDATE.
     */
-   CheckValidResultRel(leaf_part_rri, CMD_INSERT, NIL);
+   CheckValidResultRel(leaf_part_rri, CMD_INSERT,
+                       node ? node->onConflictAction : ONCONFLICT_NONE, NIL);
 
    /*
     * Open partition indices.  The user may have asked to check for conflicts

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 7c6c2c1f6e42ac6cd3c5ad17905f658003c5fa83..b0c4e2c0d32a46de687aebc0bbe34a945f43b9e6 100644 (file)
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -4811,7 +4811,8 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
        /*
         * Verify result relation is a valid target for the current operation
         */
-       CheckValidResultRel(resultRelInfo, operation, mergeActions);
+       CheckValidResultRel(resultRelInfo, operation, node->onConflictAction,
+                           mergeActions);
 
        resultRelInfo++;
        i++;

diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 10dcea037c3d0aa87a80d8409ee27f4cc98f3b53..31133514e84389fcf5a791d683cfeda9ce9bfffb 100644 (file)
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -244,6 +244,7 @@ extern bool ExecCheckPermissions(List *rangeTable,
                                 List *rteperminfos, bool ereport_on_violation);
 extern bool ExecCheckOneRelPerms(RTEPermissionInfo *perminfo);
 extern void CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation,
+                               OnConflictAction onConflictAction,
                                List *mergeActions);
 extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
                              Relation resultRelationDesc,

diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 53268059142eedc5bfe7475d067c254a2cc1fc37..00f3cb978d7743e1c2eb41ec24237d7c426f1183 100644 (file)
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -1924,6 +1924,29 @@ DROP PUBLICATION pub1;
 DROP PUBLICATION pub2;
 DROP TABLE gencols;
 RESET client_min_messages;
+-- Test that the INSERT ON CONFLICT command correctly checks REPLICA IDENTITY
+-- when the target table is published.
+CREATE TABLE testpub_insert_onconfl_no_ri (a int unique, b int);
+CREATE TABLE testpub_insert_onconfl_parted (a int unique, b int) PARTITION by RANGE (a);
+CREATE TABLE testpub_insert_onconfl_part_no_ri PARTITION OF testpub_insert_onconfl_parted FOR VALUES FROM (1) TO (10);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION pub1 FOR ALL TABLES;
+RESET client_min_messages;
+-- fail - missing REPLICA IDENTITY
+INSERT INTO testpub_insert_onconfl_no_ri VALUES (1, 1) ON CONFLICT (a) DO UPDATE SET b = 2;
+ERROR:  cannot update table "testpub_insert_onconfl_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.
+-- ok - no updates
+INSERT INTO testpub_insert_onconfl_no_ri VALUES (1, 1) ON CONFLICT DO NOTHING;
+-- fail - missing REPLICA IDENTITY in partition testpub_insert_onconfl_no_ri
+INSERT INTO testpub_insert_onconfl_parted VALUES (1, 1) ON CONFLICT (a) DO UPDATE SET b = 2;
+ERROR:  cannot update table "testpub_insert_onconfl_part_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.
+-- ok - no updates
+INSERT INTO testpub_insert_onconfl_parted VALUES (1, 1) ON CONFLICT DO NOTHING;
+DROP PUBLICATION pub1;
+DROP TABLE testpub_insert_onconfl_no_ri;
+DROP TABLE testpub_insert_onconfl_parted;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;

diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index deddf0da8445f32172b48d1a128ae628c89f6c70..53422d30320d355d194505b181d48ae375203167 100644 (file)
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -1223,6 +1223,33 @@ DROP PUBLICATION pub2;
 DROP TABLE gencols;
 
 RESET client_min_messages;
+
+-- Test that the INSERT ON CONFLICT command correctly checks REPLICA IDENTITY
+-- when the target table is published.
+CREATE TABLE testpub_insert_onconfl_no_ri (a int unique, b int);
+CREATE TABLE testpub_insert_onconfl_parted (a int unique, b int) PARTITION by RANGE (a);
+CREATE TABLE testpub_insert_onconfl_part_no_ri PARTITION OF testpub_insert_onconfl_parted FOR VALUES FROM (1) TO (10);
+
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION pub1 FOR ALL TABLES;
+RESET client_min_messages;
+
+-- fail - missing REPLICA IDENTITY
+INSERT INTO testpub_insert_onconfl_no_ri VALUES (1, 1) ON CONFLICT (a) DO UPDATE SET b = 2;
+
+-- ok - no updates
+INSERT INTO testpub_insert_onconfl_no_ri VALUES (1, 1) ON CONFLICT DO NOTHING;
+
+-- fail - missing REPLICA IDENTITY in partition testpub_insert_onconfl_no_ri
+INSERT INTO testpub_insert_onconfl_parted VALUES (1, 1) ON CONFLICT (a) DO UPDATE SET b = 2;
+
+-- ok - no updates
+INSERT INTO testpub_insert_onconfl_parted VALUES (1, 1) ON CONFLICT DO NOTHING;
+
+DROP PUBLICATION pub1;
+DROP TABLE testpub_insert_onconfl_no_ri;
+DROP TABLE testpub_insert_onconfl_parted;
+
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;