Fix permission checks on constraint violation errors on partitions.

If a cross-partition UPDATE violates a constraint on the target partition,
and the columns in the new partition are in different physical order than
in the parent, the error message can reveal columns that the user does not
have SELECT permission on. A similar bug was fixed earlier in commit
804b6b6db4.

The cause of the bug is that the callers of the
ExecBuildSlotValueDescription() function got confused when constructing
the list of modified columns. If the tuple was routed from a parent, we
converted the tuple to the parent's format, but the list of modified
columns was grabbed directly from the child's RTE entry.

ExecUpdateLockMode() had a similar issue. That lead to confusion on which
columns are key columns, leading to wrong tuple lock being taken on tables
referenced by foreign keys, when a row is updated with INSERT ON CONFLICT
UPDATE. A new isolation test is added for that corner case.

With this patch, the ri_RangeTableIndex field is no longer set for
partitions that don't have an entry in the range table. Previously, it was
set to the RTE entry of the parent relation, but that was confusing.

NOTE: This modifies the ResultRelInfo struct, replacing the
ri_PartitionRoot field with ri_RootResultRelInfo. That's a bit risky to
backpatch, because it breaks any extensions accessing the field. The
change that ri_RangeTableIndex is not set for partitions could potentially
break extensions, too. The ResultRelInfos are visible to FDWs at least,
and this patch required small changes to postgres_fdw. Nevertheless, this
seem like the least bad option. I don't think these fields widely used in
extensions; I don't think there are FDWs out there that uses the FDW
"direct update" API, other than postgres_fdw. If there is, you will get a
compilation error, so hopefully it is caught quickly.

Backpatch to 11, where support for both cross-partition UPDATEs, and unique
indexes on partitioned tables, were added.

Reviewed-by: Amit Langote
Security: CVE-2021-3393

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 105d505409f4d03f4b608ec5e58b67f3cb9851e7..f0e90d0bd3e798d589b6f6427a6b92cdb634065a 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1934,7 +1934,7 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
    PgFdwModifyState *fmstate;
    ModifyTable *plan = castNode(ModifyTable, mtstate->ps.plan);
    EState     *estate = mtstate->ps.state;
-   Index       resultRelation = resultRelInfo->ri_RangeTableIndex;
+   Index       resultRelation;
    Relation    rel = resultRelInfo->ri_RelationDesc;
    RangeTblEntry *rte;
    TupleDesc   tupdesc = RelationGetDescr(rel);
@@ -1985,7 +1985,8 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
    }
 
    /*
-    * If the foreign table is a partition, we need to create a new RTE
+    * If the foreign table is a partition that doesn't have a corresponding
+    * RTE entry, we need to create a new RTE
     * describing the foreign table for use by deparseInsertSql and
     * create_foreign_modify() below, after first copying the parent's RTE and
     * modifying some fields to describe the foreign partition to work on.
@@ -1993,9 +1994,11 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
     * correspond to this partition if it is one of the UPDATE subplan target
     * rels; in that case, we can just use the existing RTE as-is.
     */
-   rte = exec_rt_fetch(resultRelation, estate);
-   if (rte->relid != RelationGetRelid(rel))
+   if (resultRelInfo->ri_RangeTableIndex == 0)
    {
+       ResultRelInfo *rootResultRelInfo = resultRelInfo->ri_RootResultRelInfo;
+
+       rte = exec_rt_fetch(rootResultRelInfo->ri_RangeTableIndex, estate);
        rte = copyObject(rte);
        rte->relid = RelationGetRelid(rel);
        rte->relkind = RELKIND_FOREIGN_TABLE;
@@ -2007,8 +2010,15 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
         * Vars contained in those expressions.
         */
        if (plan && plan->operation == CMD_UPDATE &&
-           resultRelation == plan->rootRelation)
+           rootResultRelInfo->ri_RangeTableIndex == plan->rootRelation)
            resultRelation = mtstate->resultRelInfo[0].ri_RangeTableIndex;
+       else
+           resultRelation = rootResultRelInfo->ri_RangeTableIndex;
+   }
+   else
+   {
+       resultRelation = resultRelInfo->ri_RangeTableIndex;
+       rte = exec_rt_fetch(resultRelation, estate);
    }
 
    /* Construct the SQL command string. */

diff --git a/src/backend/access/common/tupconvert.c b/src/backend/access/common/tupconvert.c
index 5ed136246af81b766f0fb6d00f5849fe19c46b8b..37656cbedb1f9e398bc70021dfc3eeebd1b0eb51 100644 (file)
--- a/src/backend/access/common/tupconvert.c
+++ b/src/backend/access/common/tupconvert.c
@@ -493,6 +493,59 @@ execute_attr_map_slot(AttrNumber *attrMap,
    return out_slot;
 }
 
+/*
+ * Perform conversion of bitmap of columns according to the map.
+ *
+ * The input and output bitmaps are offset by
+ * FirstLowInvalidHeapAttributeNumber to accommodate system cols, like the
+ * column-bitmaps in RangeTblEntry.
+ */
+Bitmapset *
+execute_attr_map_cols(Bitmapset *in_cols, TupleConversionMap *map)
+{
+   AttrNumber *attrMap = map->attrMap;
+   int         maplen = map->outdesc->natts;
+   Bitmapset  *out_cols;
+   int         out_attnum;
+
+   /* fast path for the common trivial case */
+   if (in_cols == NULL)
+       return NULL;
+
+   /*
+    * For each output column, check which input column it corresponds to.
+    */
+   out_cols = NULL;
+
+   for (out_attnum = FirstLowInvalidHeapAttributeNumber + 1;
+        out_attnum <= maplen;
+        out_attnum++)
+   {
+       int         in_attnum;
+
+       if (out_attnum < 0)
+       {
+           /* System column. No mapping. */
+           in_attnum = out_attnum;
+       }
+       else if (out_attnum == 0)
+           continue;
+       else
+       {
+           /* normal user column */
+           in_attnum = attrMap[out_attnum - 1];
+
+           if (in_attnum == 0)
+               continue;
+       }
+
+       if (bms_is_member(in_attnum - FirstLowInvalidHeapAttributeNumber, in_cols))
+           out_cols = bms_add_member(out_cols, out_attnum - FirstLowInvalidHeapAttributeNumber);
+   }
+
+   return out_cols;
+}
+
 /*
  * Free a TupleConversionMap structure.
  */

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 34334ff622df9a0158f6ff3d586dfcc3944c4135..bd12d5ca746b9808dbaf27f84bfe0c9f36112623 100644 (file)
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2858,6 +2858,7 @@ CopyFrom(CopyState cstate)
    mtstate->ps.state = estate;
    mtstate->operation = CMD_INSERT;
    mtstate->resultRelInfo = estate->es_result_relations;
+   mtstate->rootResultRelInfo = estate->es_result_relations;
 
    if (resultRelInfo->ri_FdwRoutine != NULL &&
        resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index a40bc14ec5827a01773796cf94936ad865746e0d..ab60c73c91ae5d4528b1dddb81c608d3886bc85b 100644 (file)
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -3210,7 +3210,7 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors,
    /* Should we explicitly label target relations? */
    labeltargets = (mtstate->mt_nplans > 1 ||
                    (mtstate->mt_nplans == 1 &&
-                    mtstate->resultRelInfo->ri_RangeTableIndex != node->nominalRelation));
+                    mtstate->resultRelInfo[0].ri_RangeTableIndex != node->nominalRelation));
 
    if (labeltargets)
        ExplainOpenGroup("Target Tables", "Target Tables", false, es);

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 5d26f998fac42be398f4bea536f6923f96973076..95752fa31d8a5c963bed0c54e9937504520126fa 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -69,16 +69,6 @@ int          SessionReplicationRole = SESSION_REPLICATION_ROLE_ORIGIN;
 /* How many levels deep into trigger execution are we? */
 static int MyTriggerDepth = 0;
 
-/*
- * Note that similar macros also exist in executor/execMain.c.  There does not
- * appear to be any good header to put them into, given the structures that
- * they use, so we let them be duplicated.  Be sure to update all if one needs
- * to be changed, however.
- */
-#define GetAllUpdatedColumns(relinfo, estate) \
-   (bms_union(exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->updatedCols, \
-              exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->extraUpdatedCols))
-
 /* Local function prototypes */
 static void ConvertTriggerToFK(CreateTrigStmt *stmt, Oid funcoid);
 static void SetTriggerFlags(TriggerDesc *trigdesc, Trigger *trigger);
@@ -2928,7 +2918,10 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
                                   CMD_UPDATE))
        return;
 
-   updatedCols = GetAllUpdatedColumns(relinfo, estate);
+   /* statement-level triggers operate on the parent table */
+   Assert(relinfo->ri_RootResultRelInfo == NULL);
+
+   updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
 
    LocTriggerData.type = T_TriggerData;
    LocTriggerData.tg_event = TRIGGER_EVENT_UPDATE |
@@ -2974,10 +2967,13 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
 {
    TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
 
+   /* statement-level triggers operate on the parent table */
+   Assert(relinfo->ri_RootResultRelInfo == NULL);
+
    if (trigdesc && trigdesc->trig_update_after_statement)
        AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE,
                              false, NULL, NULL, NIL,
-                             GetAllUpdatedColumns(relinfo, estate),
+                             ExecGetAllUpdatedCols(relinfo, estate),
                              transition_capture);
 }
 
@@ -3049,7 +3045,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
    LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
    LocTriggerData.tg_oldtable = NULL;
    LocTriggerData.tg_newtable = NULL;
-   updatedCols = GetAllUpdatedColumns(relinfo, estate);
+   updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
    for (i = 0; i < trigdesc->numtriggers; i++)
    {
        Trigger    *trigger = &trigdesc->triggers[i];
@@ -3149,7 +3145,7 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
 
        AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE,
                              true, oldslot, newslot, recheckIndexes,
-                             GetAllUpdatedColumns(relinfo, estate),
+                             ExecGetAllUpdatedCols(relinfo, estate),
                              transition_capture);
    }
 }

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 2275a2988f658ab70e902dfec0e28c8bb2416ad9..88df1a124e297454f19a1fff2fa7f7e1f48270dc 100644 (file)
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -100,20 +100,6 @@ static char *ExecBuildSlotValueDescription(Oid reloid,
                                           int maxfieldlen);
 static void EvalPlanQualStart(EPQState *epqstate, Plan *planTree);
 
-/*
- * Note that GetAllUpdatedColumns() also exists in commands/trigger.c.  There does
- * not appear to be any good header to put it into, given the structures that
- * it uses, so we let them be duplicated.  Be sure to update both if one needs
- * to be changed, however.
- */
-#define GetInsertedColumns(relinfo, estate) \
-   (exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->insertedCols)
-#define GetUpdatedColumns(relinfo, estate) \
-   (exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->updatedCols)
-#define GetAllUpdatedColumns(relinfo, estate) \
-   (bms_union(exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->updatedCols, \
-              exec_rt_fetch((relinfo)->ri_RangeTableIndex, estate)->extraUpdatedCols))
-
 /* end of local decls */
 
 
@@ -1277,7 +1263,7 @@ void
 InitResultRelInfo(ResultRelInfo *resultRelInfo,
                  Relation resultRelationDesc,
                  Index resultRelationIndex,
-                 Relation partition_root,
+                 ResultRelInfo *partition_root_rri,
                  int instrument_options)
 {
    List       *partition_check = NIL;
@@ -1342,7 +1328,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
    partition_check = RelationGetPartitionQual(resultRelationDesc);
 
    resultRelInfo->ri_PartitionCheck = partition_check;
-   resultRelInfo->ri_PartitionRoot = partition_root;
+   resultRelInfo->ri_RootResultRelInfo = partition_root_rri;
    resultRelInfo->ri_PartitionInfo = NULL; /* may be set later */
    resultRelInfo->ri_CopyMultiInsertBuffer = NULL;
 }
@@ -1840,13 +1826,14 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
     * back to the root table's rowtype so that val_desc in the error message
     * matches the input tuple.
     */
-   if (resultRelInfo->ri_PartitionRoot)
+   if (resultRelInfo->ri_RootResultRelInfo)
    {
+       ResultRelInfo *rootrel = resultRelInfo->ri_RootResultRelInfo;
        TupleDesc   old_tupdesc;
        AttrNumber *map;
 
-       root_relid = RelationGetRelid(resultRelInfo->ri_PartitionRoot);
-       tupdesc = RelationGetDescr(resultRelInfo->ri_PartitionRoot);
+       root_relid = RelationGetRelid(rootrel->ri_RelationDesc);
+       tupdesc = RelationGetDescr(rootrel->ri_RelationDesc);
 
        old_tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
        /* a reverse map */
@@ -1860,16 +1847,17 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
        if (map != NULL)
            slot = execute_attr_map_slot(map, slot,
                                         MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
+       modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
+                                ExecGetUpdatedCols(rootrel, estate));
    }
    else
    {
        root_relid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
        tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
+       modifiedCols = bms_union(ExecGetInsertedCols(resultRelInfo, estate),
+                                ExecGetUpdatedCols(resultRelInfo, estate));
    }
 
-   modifiedCols = bms_union(GetInsertedColumns(resultRelInfo, estate),
-                            GetUpdatedColumns(resultRelInfo, estate));
-
    val_desc = ExecBuildSlotValueDescription(root_relid,
                                             slot,
                                             tupdesc,
@@ -1901,8 +1889,6 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
    TupleDesc   tupdesc = RelationGetDescr(rel);
    TupleConstr *constr = tupdesc->constr;
    Bitmapset  *modifiedCols;
-   Bitmapset  *insertedCols;
-   Bitmapset  *updatedCols;
 
    Assert(constr || resultRelInfo->ri_PartitionCheck);
 
@@ -1928,12 +1914,12 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
                 * rowtype so that val_desc shown error message matches the
                 * input tuple.
                 */
-               if (resultRelInfo->ri_PartitionRoot)
+               if (resultRelInfo->ri_RootResultRelInfo)
                {
+                   ResultRelInfo *rootrel = resultRelInfo->ri_RootResultRelInfo;
                    AttrNumber *map;
 
-                   rel = resultRelInfo->ri_PartitionRoot;
-                   tupdesc = RelationGetDescr(rel);
+                   tupdesc = RelationGetDescr(rootrel->ri_RelationDesc);
                    /* a reverse map */
                    map = convert_tuples_by_name_map_if_req(orig_tupdesc,
                                                            tupdesc,
@@ -1946,11 +1932,13 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
                    if (map != NULL)
                        slot = execute_attr_map_slot(map, slot,
                                                     MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
+                   modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
+                                            ExecGetUpdatedCols(rootrel, estate));
+                   rel = rootrel->ri_RelationDesc;
                }
-
-               insertedCols = GetInsertedColumns(resultRelInfo, estate);
-               updatedCols = GetUpdatedColumns(resultRelInfo, estate);
-               modifiedCols = bms_union(insertedCols, updatedCols);
+               else
+                   modifiedCols = bms_union(ExecGetInsertedCols(resultRelInfo, estate),
+                                            ExecGetUpdatedCols(resultRelInfo, estate));
                val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
                                                         slot,
                                                         tupdesc,
@@ -1977,13 +1965,13 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
            Relation    orig_rel = rel;
 
            /* See the comment above. */
-           if (resultRelInfo->ri_PartitionRoot)
+           if (resultRelInfo->ri_RootResultRelInfo)
            {
+               ResultRelInfo *rootrel = resultRelInfo->ri_RootResultRelInfo;
                TupleDesc   old_tupdesc = RelationGetDescr(rel);
                AttrNumber *map;
 
-               rel = resultRelInfo->ri_PartitionRoot;
-               tupdesc = RelationGetDescr(rel);
+               tupdesc = RelationGetDescr(rootrel->ri_RelationDesc);
                /* a reverse map */
                map = convert_tuples_by_name_map_if_req(old_tupdesc,
                                                        tupdesc,
@@ -1996,11 +1984,13 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
                if (map != NULL)
                    slot = execute_attr_map_slot(map, slot,
                                                 MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
+               modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
+                                        ExecGetUpdatedCols(rootrel, estate));
+               rel = rootrel->ri_RelationDesc;
            }
-
-           insertedCols = GetInsertedColumns(resultRelInfo, estate);
-           updatedCols = GetUpdatedColumns(resultRelInfo, estate);
-           modifiedCols = bms_union(insertedCols, updatedCols);
+           else
+               modifiedCols = bms_union(ExecGetInsertedCols(resultRelInfo, estate),
+                                        ExecGetUpdatedCols(resultRelInfo, estate));
            val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
                                                     slot,
                                                     tupdesc,
@@ -2069,8 +2059,6 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
        {
            char       *val_desc;
            Bitmapset  *modifiedCols;
-           Bitmapset  *insertedCols;
-           Bitmapset  *updatedCols;
 
            switch (wco->kind)
            {
@@ -2085,13 +2073,13 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
                     */
                case WCO_VIEW_CHECK:
                    /* See the comment in ExecConstraints(). */
-                   if (resultRelInfo->ri_PartitionRoot)
+                   if (resultRelInfo->ri_RootResultRelInfo)
                    {
+                       ResultRelInfo *rootrel = resultRelInfo->ri_RootResultRelInfo;
                        TupleDesc   old_tupdesc = RelationGetDescr(rel);
                        AttrNumber *map;
 
-                       rel = resultRelInfo->ri_PartitionRoot;
-                       tupdesc = RelationGetDescr(rel);
+                       tupdesc = RelationGetDescr(rootrel->ri_RelationDesc);
                        /* a reverse map */
                        map = convert_tuples_by_name_map_if_req(old_tupdesc,
                                                                tupdesc,
@@ -2104,11 +2092,14 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
                        if (map != NULL)
                            slot = execute_attr_map_slot(map, slot,
                                                         MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
-                   }
 
-                   insertedCols = GetInsertedColumns(resultRelInfo, estate);
-                   updatedCols = GetUpdatedColumns(resultRelInfo, estate);
-                   modifiedCols = bms_union(insertedCols, updatedCols);
+                       modifiedCols = bms_union(ExecGetInsertedCols(rootrel, estate),
+                                                ExecGetUpdatedCols(rootrel, estate));
+                       rel = rootrel->ri_RelationDesc;
+                   }
+                   else
+                       modifiedCols = bms_union(ExecGetInsertedCols(resultRelInfo, estate),
+                                                ExecGetUpdatedCols(resultRelInfo, estate));
                    val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
                                                             slot,
                                                             tupdesc,
@@ -2322,7 +2313,7 @@ ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo)
     * been modified, then we can use a weaker lock, allowing for better
     * concurrency.
     */
-   updatedCols = GetAllUpdatedColumns(relinfo, estate);
+   updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
    keyCols = RelationGetIndexAttrBitmap(relinfo->ri_RelationDesc,
                                         INDEX_ATTR_BITMAP_KEY);
 

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 93e2cdd0207c1ed10c1898d73029d7cf167d0af8..8fe4fc57bf614ba0042717a59abba19e987e4014 100644 (file)
--- a/src/backend/executor/execPartition.c