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/executor/execMain.c b/src/backend/executor/execMain.c
index 3143fda447740ccb363e7ad0513dad18c984ad21..cf3867842c99857515ddce2dadc5577cefb31cdf 100644 (file)
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -979,11 +979,15 @@ 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.
+ *
  * Note: when changing this function, you probably also need to look at
  * CheckValidRowMarkRel.
  */
 void
-CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation)
+CheckValidResultRelNew(ResultRelInfo *resultRelInfo, CmdType operation,
+                      OnConflictAction onConflictAction)
 {
    Relation    resultRel = resultRelInfo->ri_RelationDesc;
    TriggerDesc *trigDesc = resultRel->trigdesc;
@@ -998,6 +1002,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,
@@ -1116,6 +1127,16 @@ CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation)
    }
 }
 
+/*
+ * ABI-compatible wrapper to emulate old version of the above function.
+ * Do not call this version in new code.
+ */
+void
+CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation)
+{
+   return CheckValidResultRelNew(resultRelInfo, operation, ONCONFLICT_NONE);
+}
+
 /*
  * Check that a proposed rowmark target relation is a legal target
  *

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 3454438f7c2e89a50c2c218e3149d91b99d3cae2..5386e665a94a2ad9ebeae1da0f3b23f0422a36ea 100644 (file)
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -352,8 +352,11 @@ ExecFindPartition(ModifyTableState *mtstate,
                                               true, false);
                if (rri)
                {
+                   ModifyTable *node = (ModifyTable *) mtstate->ps.plan;
+
                    /* Verify this ResultRelInfo allows INSERTs */
-                   CheckValidResultRel(rri, CMD_INSERT);
+                   CheckValidResultRelNew(rri, CMD_INSERT,
+                                          node ? node->onConflictAction : ONCONFLICT_NONE);
 
                    /*
                     * Initialize information needed to insert this and
@@ -519,7 +522,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);
+   CheckValidResultRelNew(leaf_part_rri, CMD_INSERT,
+                          node ? node->onConflictAction : ONCONFLICT_NONE);
 
    /*
     * 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 3a4ce5c32f8e6deeec2747d3bd74b4764f4862b4..ae1b07847b128c9b8e31a1115086319fb9783d3e 100644 (file)
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2984,7 +2984,8 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
        /*
         * Verify result relation is a valid target for the current operation
         */
-       CheckValidResultRel(resultRelInfo, operation);
+       CheckValidResultRelNew(resultRelInfo, operation,
+                              node->onConflictAction);
 
        resultRelInfo++;
        i++;

diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 9de6b9c6c4933cc573825924d2f72b781741215c..4bb1744286aa45bb67e359dab45608ee93eba7bc 100644 (file)
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -198,6 +198,8 @@ extern void standard_ExecutorEnd(QueryDesc *queryDesc);
 extern void ExecutorRewind(QueryDesc *queryDesc);
 extern bool ExecCheckRTPerms(List *rangeTable, bool ereport_on_violation);
 extern bool ExecCheckRTEPerms(RangeTblEntry *rte);
+extern void CheckValidResultRelNew(ResultRelInfo *resultRelInfo, CmdType operation,
+                                  OnConflictAction onConflictAction);
 extern void CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation);
 extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
                              Relation resultRelationDesc,

diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 35595fb14273166191ae68a6fa988eb1610c44fc..e78d302d3bcff23dd74e64d52e479c89b6515039 100644 (file)
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -341,6 +341,29 @@ DROP PUBLICATION testpib_ins_trunct;
 DROP PUBLICATION testpub_fortbl;
 DROP SCHEMA pub_test CASCADE;
 NOTICE:  drop cascades to table pub_test.testpub_nopk
+-- 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 021d4e2cb24984b1aa3cd2be312c1a87f5305b9c..dc09d1fdf6ff492280cec7cc42ea60d600fe5880 100644 (file)
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -215,6 +215,32 @@ DROP PUBLICATION testpub_fortbl;
 
 DROP SCHEMA pub_test CASCADE;
 
+-- 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;