Rework query relation permission checking

Currently, information about the permissions to be checked on relations
mentioned in a query is stored in their range table entries.  So the
executor must scan the entire range table looking for relations that
need to have permissions checked.  This can make the permission checking
part of the executor initialization needlessly expensive when many
inheritance children are present in the range range.  While the
permissions need not be checked on the individual child relations, the
executor still must visit every range table entry to filter them out.

This commit moves the permission checking information out of the range
table entries into a new plan node called RTEPermissionInfo.  Every
top-level (inheritance "root") RTE_RELATION entry in the range table
gets one and a list of those is maintained alongside the range table.
This new list is initialized by the parser when initializing the range
table.  The rewriter can add more entries to it as rules/views are
expanded.  Finally, the planner combines the lists of the individual
subqueries into one flat list that is passed to the executor for
checking.

To make it quick to find the RTEPermissionInfo entry belonging to a
given relation, RangeTblEntry gets a new Index field 'perminfoindex'
that stores the corresponding RTEPermissionInfo's index in the query's
list of the latter.

ExecutorCheckPerms_hook has gained another List * argument; the
signature is now:
typedef bool (*ExecutorCheckPerms_hook_type) (List *rangeTable,
					      List *rtePermInfos,
					      bool ereport_on_violation);
The first argument is no longer used by any in-core uses of the hook,
but we leave it in place because there may be other implementations that
do.  Implementations should likely scan the rtePermInfos list to
determine which operations to allow or deny.

Author: Amit Langote <amitlangote09@gmail.com>
Discussion: https://postgr.es/m/CA+HiwqGjJDmUhDSfv-U2qhKJjt9ST7Xh9JXC_irsAQ1TAUsJYg@mail.gmail.com

1 files changed, 99 insertions, 79 deletions

diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 4665f0b2b7c..b4878a92eab 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1037,11 +1037,15 @@ markRTEForSelectPriv(ParseState *pstate, int rtindex, AttrNumber col)
 
 	if (rte->rtekind == RTE_RELATION)
 	{
+		RTEPermissionInfo *perminfo;
+
 		/* Make sure the rel as a whole is marked for SELECT access */
-		rte->requiredPerms |= ACL_SELECT;
+		perminfo = getRTEPermissionInfo(pstate->p_rteperminfos, rte);
+		perminfo->requiredPerms |= ACL_SELECT;
 		/* Must offset the attnum to fit in a bitmapset */
-		rte->selectedCols = bms_add_member(rte->selectedCols,
-										   col - FirstLowInvalidHeapAttributeNumber);
+		perminfo->selectedCols =
+			bms_add_member(perminfo->selectedCols,
+						   col - FirstLowInvalidHeapAttributeNumber);
 	}
 	else if (rte->rtekind == RTE_JOIN)
 	{
@@ -1251,10 +1255,13 @@ chooseScalarFunctionAlias(Node *funcexpr, char *funcname,
  *
  * rte: the new RangeTblEntry for the rel
  * rtindex: its index in the rangetable list
+ * perminfo: permission list entry for the rel
  * tupdesc: the physical column information
  */
 static ParseNamespaceItem *
-buildNSItemFromTupleDesc(RangeTblEntry *rte, Index rtindex, TupleDesc tupdesc)
+buildNSItemFromTupleDesc(RangeTblEntry *rte, Index rtindex,
+						 RTEPermissionInfo *perminfo,
+						 TupleDesc tupdesc)
 {
 	ParseNamespaceItem *nsitem;
 	ParseNamespaceColumn *nscolumns;
@@ -1290,6 +1297,7 @@ buildNSItemFromTupleDesc(RangeTblEntry *rte, Index rtindex, TupleDesc tupdesc)
 	nsitem->p_names = rte->eref;
 	nsitem->p_rte = rte;
 	nsitem->p_rtindex = rtindex;
+	nsitem->p_perminfo = perminfo;
 	nsitem->p_nscolumns = nscolumns;
 	/* set default visibility flags; might get changed later */
 	nsitem->p_rel_visible = true;
@@ -1433,6 +1441,7 @@ addRangeTableEntry(ParseState *pstate,
 				   bool inFromCl)
 {
 	RangeTblEntry *rte = makeNode(RangeTblEntry);
+	RTEPermissionInfo *perminfo;
 	char	   *refname = alias ? alias->aliasname : relation->relname;
 	LOCKMODE	lockmode;
 	Relation	rel;
@@ -1469,7 +1478,7 @@ addRangeTableEntry(ParseState *pstate,
 	buildRelationAliases(rel->rd_att, alias, rte->eref);
 
 	/*
-	 * Set flags and access permissions.
+	 * Set flags and initialize access permissions.
 	 *
 	 * The initial default on access checks is always check-for-READ-access,
 	 * which is the right thing for all except target tables.
@@ -1478,12 +1487,8 @@ addRangeTableEntry(ParseState *pstate,
 	rte->inh = inh;
 	rte->inFromCl = inFromCl;
 
-	rte->requiredPerms = ACL_SELECT;
-	rte->checkAsUser = InvalidOid;	/* not set-uid by default, either */
-	rte->selectedCols = NULL;
-	rte->insertedCols = NULL;
-	rte->updatedCols = NULL;
-	rte->extraUpdatedCols = NULL;
+	perminfo = addRTEPermissionInfo(&pstate->p_rteperminfos, rte);
+	perminfo->requiredPerms = ACL_SELECT;
 
 	/*
 	 * Add completed RTE to pstate's range table list, so that we know its
@@ -1497,7 +1502,7 @@ addRangeTableEntry(ParseState *pstate,
 	 * list --- caller must do that if appropriate.
 	 */
 	nsitem = buildNSItemFromTupleDesc(rte, list_length(pstate->p_rtable),
-									  rel->rd_att);
+									  perminfo, rel->rd_att);
 
 	/*
 	 * Drop the rel refcount, but keep the access lock till end of transaction
@@ -1534,6 +1539,7 @@ addRangeTableEntryForRelation(ParseState *pstate,
 							  bool inFromCl)
 {
 	RangeTblEntry *rte = makeNode(RangeTblEntry);
+	RTEPermissionInfo *perminfo;
 	char	   *refname = alias ? alias->aliasname : RelationGetRelationName(rel);
 
 	Assert(pstate != NULL);
@@ -1557,7 +1563,7 @@ addRangeTableEntryForRelation(ParseState *pstate,
 	buildRelationAliases(rel->rd_att, alias, rte->eref);
 
 	/*
-	 * Set flags and access permissions.
+	 * Set flags and initialize access permissions.
 	 *
 	 * The initial default on access checks is always check-for-READ-access,
 	 * which is the right thing for all except target tables.
@@ -1566,12 +1572,8 @@ addRangeTableEntryForRelation(ParseState *pstate,
 	rte->inh = inh;
 	rte->inFromCl = inFromCl;
 
-	rte->requiredPerms = ACL_SELECT;
-	rte->checkAsUser = InvalidOid;	/* not set-uid by default, either */
-	rte->selectedCols = NULL;
-	rte->insertedCols = NULL;
-	rte->updatedCols = NULL;
-	rte->extraUpdatedCols = NULL;
+	perminfo = addRTEPermissionInfo(&pstate->p_rteperminfos, rte);
+	perminfo->requiredPerms = ACL_SELECT;
 
 	/*
 	 * Add completed RTE to pstate's range table list, so that we know its
@@ -1585,7 +1587,7 @@ addRangeTableEntryForRelation(ParseState *pstate,
 	 * list --- caller must do that if appropriate.
 	 */
 	return buildNSItemFromTupleDesc(rte, list_length(pstate->p_rtable),
-									rel->rd_att);
+									perminfo, rel->rd_att);
 }
 
 /*
@@ -1659,21 +1661,15 @@ addRangeTableEntryForSubquery(ParseState *pstate,
 	rte->eref = eref;
 
 	/*
-	 * Set flags and access permissions.
+	 * Set flags.
 	 *
-	 * Subqueries are never checked for access rights.
+	 * Subqueries are never checked for access rights, so no need to perform
+	 * addRTEPermissionInfo().
 	 */
 	rte->lateral = lateral;
 	rte->inh = false;			/* never true for subqueries */
 	rte->inFromCl = inFromCl;
 
-	rte->requiredPerms = 0;
-	rte->checkAsUser = InvalidOid;
-	rte->selectedCols = NULL;
-	rte->insertedCols = NULL;
-	rte->updatedCols = NULL;
-	rte->extraUpdatedCols = NULL;
-
 	/*
 	 * Add completed RTE to pstate's range table list, so that we know its
 	 * index.  But we don't add it to the join list --- caller must do that if
@@ -1990,20 +1986,13 @@ addRangeTableEntryForFunction(ParseState *pstate,
 	/*
 	 * Set flags and access permissions.
 	 *
-	 * Functions are never checked for access rights (at least, not by the RTE
-	 * permissions mechanism).
+	 * Functions are never checked for access rights (at least, not by
+	 * ExecCheckPermissions()), so no need to perform addRTEPermissionInfo().
 	 */
 	rte->lateral = lateral;
 	rte->inh = false;			/* never true for functions */
 	rte->inFromCl = inFromCl;
 
-	rte->requiredPerms = 0;
-	rte->checkAsUser = InvalidOid;
-	rte->selectedCols = NULL;
-	rte->insertedCols = NULL;
-	rte->updatedCols = NULL;
-	rte->extraUpdatedCols = NULL;
-
 	/*
 	 * Add completed RTE to pstate's range table list, so that we know its
 	 * index.  But we don't add it to the join list --- caller must do that if
@@ -2015,7 +2004,7 @@ addRangeTableEntryForFunction(ParseState *pstate,
 	 * Build a ParseNamespaceItem, but don't add it to the pstate's namespace
 	 * list --- caller must do that if appropriate.
 	 */
-	return buildNSItemFromTupleDesc(rte, list_length(pstate->p_rtable),
+	return buildNSItemFromTupleDesc(rte, list_length(pstate->p_rtable), NULL,
 									tupdesc);
 }
 
@@ -2082,20 +2071,13 @@ addRangeTableEntryForTableFunc(ParseState *pstate,
 	/*
 	 * Set flags and access permissions.
 	 *
-	 * Tablefuncs are never checked for access rights (at least, not by the
-	 * RTE permissions mechanism).
+	 * Tablefuncs are never checked for access rights (at least, not by
+	 * ExecCheckPermissions()), so no need to perform addRTEPermissionInfo().
 	 */
 	rte->lateral = lateral;
 	rte->inh = false;			/* never true for tablefunc RTEs */
 	rte->inFromCl = inFromCl;
 
-	rte->requiredPerms = 0;
-	rte->checkAsUser = InvalidOid;
-	rte->selectedCols = NULL;
-	rte->insertedCols = NULL;
-	rte->updatedCols = NULL;
-	rte->extraUpdatedCols = NULL;
-
 	/*
 	 * Add completed RTE to pstate's range table list, so that we know its
 	 * index.  But we don't add it to the join list --- caller must do that if
@@ -2170,19 +2152,13 @@ addRangeTableEntryForValues(ParseState *pstate,
 	/*
 	 * Set flags and access permissions.
 	 *
-	 * Subqueries are never checked for access rights.
+	 * Subqueries are never checked for access rights, so no need to perform
+	 * addRTEPermissionInfo().
 	 */
 	rte->lateral = lateral;
 	rte->inh = false;			/* never true for values RTEs */
 	rte->inFromCl = inFromCl;
 
-	rte->requiredPerms = 0;
-	rte->checkAsUser = InvalidOid;
-	rte->selectedCols = NULL;
-	rte->insertedCols = NULL;
-	rte->updatedCols = NULL;
-	rte->extraUpdatedCols = NULL;
-
 	/*
 	 * Add completed RTE to pstate's range table list, so that we know its
 	 * index.  But we don't add it to the join list --- caller must do that if
@@ -2267,19 +2243,13 @@ addRangeTableEntryForJoin(ParseState *pstate,
 	/*
 	 * Set flags and access permissions.
 	 *
-	 * Joins are never checked for access rights.
+	 * Joins are never checked for access rights, so no need to perform
+	 * addRTEPermissionInfo().
 	 */
 	rte->lateral = false;
 	rte->inh = false;			/* never true for joins */
 	rte->inFromCl = inFromCl;
 
-	rte->requiredPerms = 0;
-	rte->checkAsUser = InvalidOid;
-	rte->selectedCols = NULL;
-	rte->insertedCols = NULL;
-	rte->updatedCols = NULL;
-	rte->extraUpdatedCols = NULL;
-
 	/*
 	 * Add completed RTE to pstate's range table list, so that we know its
 	 * index.  But we don't add it to the join list --- caller must do that if
@@ -2294,6 +2264,7 @@ addRangeTableEntryForJoin(ParseState *pstate,
 	nsitem = (ParseNamespaceItem *) palloc(sizeof(ParseNamespaceItem));
 	nsitem->p_names = rte->eref;
 	nsitem->p_rte = rte;
+	nsitem->p_perminfo = NULL;
 	nsitem->p_rtindex = list_length(pstate->p_rtable);
 	nsitem->p_nscolumns = nscolumns;
 	/* set default visibility flags; might get changed later */
@@ -2417,19 +2388,13 @@ addRangeTableEntryForCTE(ParseState *pstate,
 	/*
 	 * Set flags and access permissions.
 	 *
-	 * Subqueries are never checked for access rights.
+	 * Subqueries are never checked for access rights, so no need to perform
+	 * addRTEPermissionInfo().
 	 */
 	rte->lateral = false;
 	rte->inh = false;			/* never true for subqueries */
 	rte->inFromCl = inFromCl;
 
-	rte->requiredPerms = 0;
-	rte->checkAsUser = InvalidOid;
-	rte->selectedCols = NULL;
-	rte->insertedCols = NULL;
-	rte->updatedCols = NULL;
-	rte->extraUpdatedCols = NULL;
-
 	/*
 	 * Add completed RTE to pstate's range table list, so that we know its
 	 * index.  But we don't add it to the join list --- caller must do that if
@@ -2543,16 +2508,13 @@ addRangeTableEntryForENR(ParseState *pstate,
 	/*
 	 * Set flags and access permissions.
 	 *
-	 * ENRs are never checked for access rights.
+	 * ENRs are never checked for access rights, so no need to perform
+	 * addRTEPermissionInfo().
 	 */
 	rte->lateral = false;
 	rte->inh = false;			/* never true for ENRs */
 	rte->inFromCl = inFromCl;
 
-	rte->requiredPerms = 0;
-	rte->checkAsUser = InvalidOid;
-	rte->selectedCols = NULL;
-
 	/*
 	 * Add completed RTE to pstate's range table list, so that we know its
 	 * index.  But we don't add it to the join list --- caller must do that if
@@ -2564,7 +2526,7 @@ addRangeTableEntryForENR(ParseState *pstate,
 	 * Build a ParseNamespaceItem, but don't add it to the pstate's namespace
 	 * list --- caller must do that if appropriate.
 	 */
-	return buildNSItemFromTupleDesc(rte, list_length(pstate->p_rtable),
+	return buildNSItemFromTupleDesc(rte, list_length(pstate->p_rtable), NULL,
 									tupdesc);
 }
 
@@ -3189,6 +3151,7 @@ expandNSItemAttrs(ParseState *pstate, ParseNamespaceItem *nsitem,
 				  int sublevels_up, bool require_col_privs, int location)
 {
 	RangeTblEntry *rte = nsitem->p_rte;
+	RTEPermissionInfo *perminfo = nsitem->p_perminfo;
 	List	   *names,
 			   *vars;
 	ListCell   *name,
@@ -3206,7 +3169,10 @@ expandNSItemAttrs(ParseState *pstate, ParseNamespaceItem *nsitem,
 	 * relation of UPDATE/DELETE, which cannot be under a join.)
 	 */
 	if (rte->rtekind == RTE_RELATION)
-		rte->requiredPerms |= ACL_SELECT;
+	{
+		Assert(perminfo != NULL);
+		perminfo->requiredPerms |= ACL_SELECT;
+	}
 
 	forboth(name, names, var, vars)
 	{
@@ -3855,3 +3821,57 @@ isQueryUsingTempRelation_walker(Node *node, void *context)
 								  isQueryUsingTempRelation_walker,
 								  context);
 }
+
+/*
+ * addRTEPermissionInfo
+ *		Creates RTEPermissionInfo for a given RTE and adds it into the
+ *		provided list.
+ *
+ * Returns the RTEPermissionInfo and sets rte->perminfoindex.
+ */
+RTEPermissionInfo *
+addRTEPermissionInfo(List **rteperminfos, RangeTblEntry *rte)
+{
+	RTEPermissionInfo *perminfo;
+
+	Assert(rte->rtekind == RTE_RELATION);
+	Assert(rte->perminfoindex == 0);
+
+	/* Nope, so make one and add to the list. */
+	perminfo = makeNode(RTEPermissionInfo);
+	perminfo->relid = rte->relid;
+	perminfo->inh = rte->inh;
+	/* Other information is set by fetching the node as and where needed. */
+
+	*rteperminfos = lappend(*rteperminfos, perminfo);
+
+	/* Note its index (1-based!) */
+	rte->perminfoindex = list_length(*rteperminfos);
+
+	return perminfo;
+}
+
+/*
+ * getRTEPermissionInfo
+ *		Find RTEPermissionInfo for a given relation in the provided list.
+ *
+ * This is a simple list_nth() operation, though it's good to have the
+ * function for the various sanity checks.
+ */
+RTEPermissionInfo *
+getRTEPermissionInfo(List *rteperminfos, RangeTblEntry *rte)
+{
+	RTEPermissionInfo *perminfo;
+
+	if (rte->perminfoindex == 0 ||
+		rte->perminfoindex > list_length(rteperminfos))
+		elog(ERROR, "invalid perminfoindex %d in RTE with relid %u",
+			 rte->perminfoindex, rte->relid);
+	perminfo = list_nth_node(RTEPermissionInfo, rteperminfos,
+							 rte->perminfoindex - 1);
+	if (perminfo->relid != rte->relid)
+		elog(ERROR, "permission info at index %u (with relid=%u) does not match provided RTE (with relid=%u)",
+			 rte->perminfoindex, perminfo->relid, rte->relid);
+
+	return perminfo;
+}