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, 108 insertions, 97 deletions

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 1e3efbbd36f..ea56ff79c86 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -395,36 +395,43 @@ rewriteRuleAction(Query *parsetree,
 	 * Generate expanded rtable consisting of main parsetree's rtable plus
 	 * rule action's rtable; this becomes the complete rtable for the rule
 	 * action.  Some of the entries may be unused after we finish rewriting,
-	 * but we leave them all in place for two reasons:
+	 * but we leave them all in place to avoid having to adjust the query's
+	 * varnos.  RT entries that are not referenced in the completed jointree
+	 * will be ignored by the planner, so they do not affect query semantics.
 	 *
-	 * We'd have a much harder job to adjust the query's varnos if we
-	 * selectively removed RT entries.
+	 * Also merge RTEPermissionInfo lists to ensure that all permissions are
+	 * checked correctly.
 	 *
 	 * If the rule is INSTEAD, then the original query won't be executed at
-	 * all, and so its rtable must be preserved so that the executor will do
-	 * the correct permissions checks on it.
+	 * all, and so its rteperminfos must be preserved so that the executor
+	 * will do the correct permissions checks on the relations referenced in
+	 * it. This allows us to check that the caller has, say, insert-permission
+	 * on a view, when the view is not semantically referenced at all in the
+	 * resulting query.
 	 *
-	 * RT entries that are not referenced in the completed jointree will be
-	 * ignored by the planner, so they do not affect query semantics.  But any
-	 * permissions checks specified in them will be applied during executor
-	 * startup (see ExecCheckRTEPerms()).  This allows us to check that the
-	 * caller has, say, insert-permission on a view, when the view is not
-	 * semantically referenced at all in the resulting query.
+	 * When a rule is not INSTEAD, the permissions checks done using the
+	 * copied entries will be redundant with those done during execution of
+	 * the original query, but we don't bother to treat that case differently.
 	 *
-	 * When a rule is not INSTEAD, the permissions checks done on its copied
-	 * RT entries will be redundant with those done during execution of the
-	 * original query, but we don't bother to treat that case differently.
-	 *
-	 * NOTE: because planner will destructively alter rtable, we must ensure
-	 * that rule action's rtable is separate and shares no substructure with
-	 * the main rtable.  Hence do a deep copy here.
-	 *
-	 * Note also that RewriteQuery() relies on the fact that RT entries from
-	 * the original query appear at the start of the expanded rtable, so
-	 * beware of changing this.
+	 * NOTE: because planner will destructively alter rtable and rteperminfos,
+	 * we must ensure that rule action's lists are separate and shares no
+	 * substructure with the main query's lists.  Hence do a deep copy here
+	 * for both.
 	 */
-	sub_action->rtable = list_concat(copyObject(parsetree->rtable),
-									 sub_action->rtable);
+	{
+		List	   *rtable_tail = sub_action->rtable;
+		List	   *perminfos_tail = sub_action->rteperminfos;
+
+		/*
+		 * RewriteQuery relies on the fact that RT entries from the original
+		 * query appear at the start of the expanded rtable, so we put the
+		 * action's original table at the end of the list.
+		 */
+		sub_action->rtable = copyObject(parsetree->rtable);
+		sub_action->rteperminfos = copyObject(parsetree->rteperminfos);
+		CombineRangeTables(&sub_action->rtable, &sub_action->rteperminfos,
+						   rtable_tail, perminfos_tail);
+	}
 
 	/*
 	 * There could have been some SubLinks in parsetree's rtable, in which
@@ -1628,10 +1635,13 @@ rewriteValuesRTEToNulls(Query *parsetree, RangeTblEntry *rte)
 
 /*
  * Record in target_rte->extraUpdatedCols the indexes of any generated columns
- * that depend on any columns mentioned in target_rte->updatedCols.
+ * columns that depend on any columns mentioned in
+ * target_perminfo->updatedCols.
  */
 void
-fill_extraUpdatedCols(RangeTblEntry *target_rte, Relation target_relation)
+fill_extraUpdatedCols(RangeTblEntry *target_rte,
+					  RTEPermissionInfo *target_perminfo,
+					  Relation target_relation)
 {
 	TupleDesc	tupdesc = RelationGetDescr(target_relation);
 	TupleConstr *constr = tupdesc->constr;
@@ -1654,7 +1664,7 @@ fill_extraUpdatedCols(RangeTblEntry *target_rte, Relation target_relation)
 			expr = stringToNode(defval->adbin);
 			pull_varattnos(expr, 1, &attrs_used);
 
-			if (bms_overlap(target_rte->updatedCols, attrs_used))
+			if (bms_overlap(target_perminfo->updatedCols, attrs_used))
 				target_rte->extraUpdatedCols =
 					bms_add_member(target_rte->extraUpdatedCols,
 								   defval->adnum - FirstLowInvalidHeapAttributeNumber);
@@ -1747,6 +1757,8 @@ ApplyRetrieveRule(Query *parsetree,
 	Query	   *rule_action;
 	RangeTblEntry *rte,
 			   *subrte;
+	RTEPermissionInfo *perminfo,
+			   *sub_perminfo;
 	RowMarkClause *rc;
 
 	if (list_length(rule->actions) != 1)
@@ -1788,18 +1800,6 @@ ApplyRetrieveRule(Query *parsetree,
 			parsetree->resultRelation = list_length(parsetree->rtable);
 
 			/*
-			 * There's no need to do permissions checks twice, so wipe out the
-			 * permissions info for the original RTE (we prefer to keep the
-			 * bits set on the result RTE).
-			 */
-			rte->requiredPerms = 0;
-			rte->checkAsUser = InvalidOid;
-			rte->selectedCols = NULL;
-			rte->insertedCols = NULL;
-			rte->updatedCols = NULL;
-			rte->extraUpdatedCols = NULL;
-
-			/*
 			 * For the most part, Vars referencing the view should remain as
 			 * they are, meaning that they implicitly represent OLD values.
 			 * But in the RETURNING list if any, we want such Vars to
@@ -1862,12 +1862,6 @@ ApplyRetrieveRule(Query *parsetree,
 
 	/*
 	 * Recursively expand any view references inside the view.
-	 *
-	 * Note: this must happen after markQueryForLocking.  That way, any UPDATE
-	 * permission bits needed for sub-views are initially applied to their
-	 * RTE_RELATION RTEs by markQueryForLocking, and then transferred to their
-	 * OLD rangetable entries by the action below (in a recursive call of this
-	 * routine).
 	 */
 	rule_action = fireRIRrules(rule_action, activeRIRs);
 
@@ -1876,6 +1870,7 @@ ApplyRetrieveRule(Query *parsetree,
 	 * original RTE to a subquery RTE.
 	 */
 	rte = rt_fetch(rt_index, parsetree->rtable);
+	perminfo = getRTEPermissionInfo(parsetree->rteperminfos, rte);
 
 	rte->rtekind = RTE_SUBQUERY;
 	rte->subquery = rule_action;
@@ -1885,6 +1880,7 @@ ApplyRetrieveRule(Query *parsetree,
 	rte->relkind = 0;
 	rte->rellockmode = 0;
 	rte->tablesample = NULL;
+	rte->perminfoindex = 0;		/* no permission checking for this RTE */
 	rte->inh = false;			/* must not be set for a subquery */
 
 	/*
@@ -1893,19 +1889,12 @@ ApplyRetrieveRule(Query *parsetree,
 	 */
 	subrte = rt_fetch(PRS2_OLD_VARNO, rule_action->rtable);
 	Assert(subrte->relid == relation->rd_id);
-	subrte->requiredPerms = rte->requiredPerms;
-	subrte->checkAsUser = rte->checkAsUser;
-	subrte->selectedCols = rte->selectedCols;
-	subrte->insertedCols = rte->insertedCols;
-	subrte->updatedCols = rte->updatedCols;
-	subrte->extraUpdatedCols = rte->extraUpdatedCols;
-
-	rte->requiredPerms = 0;		/* no permission check on subquery itself */
-	rte->checkAsUser = InvalidOid;
-	rte->selectedCols = NULL;
-	rte->insertedCols = NULL;
-	rte->updatedCols = NULL;
-	rte->extraUpdatedCols = NULL;
+	sub_perminfo = getRTEPermissionInfo(rule_action->rteperminfos, subrte);
+	sub_perminfo->requiredPerms = perminfo->requiredPerms;
+	sub_perminfo->checkAsUser = perminfo->checkAsUser;
+	sub_perminfo->selectedCols = perminfo->selectedCols;
+	sub_perminfo->insertedCols = perminfo->insertedCols;
+	sub_perminfo->updatedCols = perminfo->updatedCols;
 
 	return parsetree;
 }
@@ -1935,8 +1924,12 @@ markQueryForLocking(Query *qry, Node *jtnode,
 
 		if (rte->rtekind == RTE_RELATION)
 		{
+			RTEPermissionInfo *perminfo;
+
 			applyLockingClause(qry, rti, strength, waitPolicy, pushedDown);
-			rte->requiredPerms |= ACL_SELECT_FOR_UPDATE;
+
+			perminfo = getRTEPermissionInfo(qry->rteperminfos, rte);
+			perminfo->requiredPerms |= ACL_SELECT_FOR_UPDATE;
 		}
 		else if (rte->rtekind == RTE_SUBQUERY)
 		{
@@ -3077,6 +3070,9 @@ rewriteTargetView(Query *parsetree, Relation view)
 	RangeTblEntry *base_rte;
 	RangeTblEntry *view_rte;
 	RangeTblEntry *new_rte;
+	RTEPermissionInfo *base_perminfo;
+	RTEPermissionInfo *view_perminfo;
+	RTEPermissionInfo *new_perminfo;
 	Relation	base_rel;
 	List	   *view_targetlist;
 	ListCell   *lc;
@@ -3213,6 +3209,7 @@ rewriteTargetView(Query *parsetree, Relation view)
 	base_rt_index = rtr->rtindex;
 	base_rte = rt_fetch(base_rt_index, viewquery->rtable);
 	Assert(base_rte->rtekind == RTE_RELATION);
+	base_perminfo = getRTEPermissionInfo(viewquery->rteperminfos, base_rte);
 
 	/*
 	 * Up to now, the base relation hasn't been touched at all in our query.
@@ -3284,57 +3281,69 @@ rewriteTargetView(Query *parsetree, Relation view)
 				   0);
 
 	/*
-	 * If the view has "security_invoker" set, mark the new target RTE for the
-	 * permissions checks that we want to enforce against the query caller.
-	 * Otherwise we want to enforce them against the view owner.
+	 * If the view has "security_invoker" set, mark the new target relation
+	 * for the permissions checks that we want to enforce against the query
+	 * caller. Otherwise we want to enforce them against the view owner.
 	 *
 	 * At the relation level, require the same INSERT/UPDATE/DELETE
 	 * permissions that the query caller needs against the view.  We drop the
-	 * ACL_SELECT bit that is presumably in new_rte->requiredPerms initially.
+	 * ACL_SELECT bit that is presumably in new_perminfo->requiredPerms
+	 * initially.
 	 *
-	 * Note: the original view RTE remains in the query's rangetable list.
-	 * Although it will be unused in the query plan, we need it there so that
-	 * the executor still performs appropriate permissions checks for the
-	 * query caller's use of the view.
+	 * Note: the original view's RTEPermissionInfo remains in the query's
+	 * rteperminfos so that the executor still performs appropriate
+	 * permissions checks for the query caller's use of the view.
 	 */
+	view_perminfo = getRTEPermissionInfo(parsetree->rteperminfos, view_rte);
+
+	/*
+	 * Disregard the perminfo in viewquery->rteperminfos that the base_rte
+	 * would currently be pointing at, because we'd like it to point now to a
+	 * new one that will be filled below.  Must set perminfoindex to 0 to not
+	 * trip over the Assert in addRTEPermissionInfo().
+	 */
+	new_rte->perminfoindex = 0;
+	new_perminfo = addRTEPermissionInfo(&parsetree->rteperminfos, new_rte);
 	if (RelationHasSecurityInvoker(view))
-		new_rte->checkAsUser = InvalidOid;
+		new_perminfo->checkAsUser = InvalidOid;
 	else
-		new_rte->checkAsUser = view->rd_rel->relowner;
-
-	new_rte->requiredPerms = view_rte->requiredPerms;
+		new_perminfo->checkAsUser = view->rd_rel->relowner;
+	new_perminfo->requiredPerms = view_perminfo->requiredPerms;
 
 	/*
 	 * Now for the per-column permissions bits.
 	 *
-	 * Initially, new_rte contains selectedCols permission check bits for all
-	 * base-rel columns referenced by the view, but since the view is a SELECT
-	 * query its insertedCols/updatedCols is empty.  We set insertedCols and
-	 * updatedCols to include all the columns the outer query is trying to
-	 * modify, adjusting the column numbers as needed.  But we leave
-	 * selectedCols as-is, so the view owner must have read permission for all
-	 * columns used in the view definition, even if some of them are not read
-	 * by the outer query.  We could try to limit selectedCols to only columns
-	 * used in the transformed query, but that does not correspond to what
-	 * happens in ordinary SELECT usage of a view: all referenced columns must
-	 * have read permission, even if optimization finds that some of them can
-	 * be discarded during query transformation.  The flattening we're doing
-	 * here is an optional optimization, too.  (If you are unpersuaded and
-	 * want to change this, note that applying adjust_view_column_set to
-	 * view_rte->selectedCols is clearly *not* the right answer, since that
-	 * neglects base-rel columns used in the view's WHERE quals.)
+	 * Initially, new_perminfo (base_perminfo) contains selectedCols
+	 * permission check bits for all base-rel columns referenced by the view,
+	 * but since the view is a SELECT query its insertedCols/updatedCols is
+	 * empty.  We set insertedCols and updatedCols to include all the columns
+	 * the outer query is trying to modify, adjusting the column numbers as
+	 * needed.  But we leave selectedCols as-is, so the view owner must have
+	 * read permission for all columns used in the view definition, even if
+	 * some of them are not read by the outer query.  We could try to limit
+	 * selectedCols to only columns used in the transformed query, but that
+	 * does not correspond to what happens in ordinary SELECT usage of a view:
+	 * all referenced columns must have read permission, even if optimization
+	 * finds that some of them can be discarded during query transformation.
+	 * The flattening we're doing here is an optional optimization, too.  (If
+	 * you are unpersuaded and want to change this, note that applying
+	 * adjust_view_column_set to view_perminfo->selectedCols is clearly *not*
+	 * the right answer, since that neglects base-rel columns used in the
+	 * view's WHERE quals.)
 	 *
 	 * This step needs the modified view targetlist, so we have to do things
 	 * in this order.
 	 */
-	Assert(bms_is_empty(new_rte->insertedCols) &&
-		   bms_is_empty(new_rte->updatedCols));
+	Assert(bms_is_empty(new_perminfo->insertedCols) &&
+		   bms_is_empty(new_perminfo->updatedCols));
+
+	new_perminfo->selectedCols = base_perminfo->selectedCols;
 
-	new_rte->insertedCols = adjust_view_column_set(view_rte->insertedCols,
-												   view_targetlist);
+	new_perminfo->insertedCols =
+		adjust_view_column_set(view_perminfo->insertedCols, view_targetlist);
 
-	new_rte->updatedCols = adjust_view_column_set(view_rte->updatedCols,
-												  view_targetlist);
+	new_perminfo->updatedCols =
+		adjust_view_column_set(view_perminfo->updatedCols, view_targetlist);
 
 	/*
 	 * Move any security barrier quals from the view RTE onto the new target
@@ -3438,7 +3447,7 @@ rewriteTargetView(Query *parsetree, Relation view)
 		 * from the view, hence we need a new column alias list).  This should
 		 * match transformOnConflictClause.  In particular, note that the
 		 * relkind is set to composite to signal that we're not dealing with
-		 * an actual relation, and no permissions checks are wanted.
+		 * an actual relation.
 		 */
 		old_exclRelIndex = parsetree->onConflict->exclRelIndex;
 
@@ -3449,8 +3458,8 @@ rewriteTargetView(Query *parsetree, Relation view)
 													   false, false);
 		new_exclRte = new_exclNSItem->p_rte;
 		new_exclRte->relkind = RELKIND_COMPOSITE_TYPE;
-		new_exclRte->requiredPerms = 0;
-		/* other permissions fields in new_exclRte are already empty */
+		/* Ignore the RTEPermissionInfo that would've been added. */
+		new_exclRte->perminfoindex = 0;
 
 		parsetree->rtable = lappend(parsetree->rtable, new_exclRte);
 		new_exclRelIndex = parsetree->onConflict->exclRelIndex =
@@ -3728,6 +3737,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
 	{
 		int			result_relation;
 		RangeTblEntry *rt_entry;
+		RTEPermissionInfo *rt_perminfo;
 		Relation	rt_entry_relation;
 		List	   *locks;
 		int			product_orig_rt_length;
@@ -3740,6 +3750,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
 		Assert(result_relation != 0);
 		rt_entry = rt_fetch(result_relation, parsetree->rtable);
 		Assert(rt_entry->rtekind == RTE_RELATION);
+		rt_perminfo = getRTEPermissionInfo(parsetree->rteperminfos, rt_entry);
 
 		/*
 		 * We can use NoLock here since either the parser or
@@ -3833,7 +3844,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
 									NULL, 0, NULL);
 
 			/* Also populate extraUpdatedCols (for generated columns) */
-			fill_extraUpdatedCols(rt_entry, rt_entry_relation);
+			fill_extraUpdatedCols(rt_entry, rt_perminfo, rt_entry_relation);
 		}
 		else if (event == CMD_MERGE)
 		{