From 599b33b9492dfefd1219c1d31801f40b3ba90b0d Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Wed, 30 Nov 2022 12:07:03 +0100 Subject: [PATCH] Stop accessing checkAsUser via RTE in some cases A future commit will move the checkAsUser field from RangeTblEntry to a new node that, unlike RTEs, will only be created for tables mentioned in the query but not for the inheritance child relations added to the query by the planner. So, checkAsUser value for a given child relation will have to be obtained by referring to that for its ancestor mentioned in the query. In preparation, it seems better to expand the use of RelOptInfo.userid during planning in place of rte->checkAsUser so that there will be fewer places to adjust for the above change. Given that the child-to-ancestor mapping is not available during the execution of a given "child" ForeignScan node, add a checkAsUser field to ForeignScan to carry the child relation's RelOptInfo.userid. Author: Amit Langote Discussion: https://postgr.es/m/CA+HiwqGFCs2uq7VRKi7g+FFKbP6Ea_2_HkgZb2HPhUfaAKT3ng@mail.gmail.com --- contrib/postgres_fdw/postgres_fdw.c | 18 ++++++--------- src/backend/executor/execMain.c | 2 +- src/backend/optimizer/plan/createplan.c | 6 ++++- src/backend/rewrite/rowsecurity.c | 2 +- src/backend/statistics/extended_stats.c | 6 ++--- src/backend/utils/adt/selfuncs.c | 29 +++++++++++++++---------- src/backend/utils/misc/rls.c | 2 +- src/include/nodes/pathnodes.h | 2 +- src/include/nodes/plannodes.h | 2 ++ 9 files changed, 39 insertions(+), 30 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 8d7500abfb..20c7b1ad05 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -624,7 +624,6 @@ postgresGetForeignRelSize(PlannerInfo *root, { PgFdwRelationInfo *fpinfo; ListCell *lc; - RangeTblEntry *rte = planner_rt_fetch(baserel->relid, root); /* * We use PgFdwRelationInfo to pass various information to subsequent @@ -663,8 +662,9 @@ postgresGetForeignRelSize(PlannerInfo *root, */ if (fpinfo->use_remote_estimate) { - Oid userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + Oid userid; + userid = OidIsValid(baserel->userid) ? baserel->userid : GetUserId(); fpinfo->user = GetUserMapping(userid, fpinfo->server->serverid); } else @@ -1510,16 +1510,14 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags) /* * Identify which user to do the remote access as. This should match what - * ExecCheckRTEPerms() does. In case of a join or aggregate, use the - * lowest-numbered member RTE as a representative; we would get the same - * result from any. + * ExecCheckRTEPerms() does. */ + userid = OidIsValid(fsplan->checkAsUser) ? fsplan->checkAsUser : GetUserId(); if (fsplan->scan.scanrelid > 0) rtindex = fsplan->scan.scanrelid; else rtindex = bms_next_member(fsplan->fs_relids, -1); rte = exec_rt_fetch(rtindex, estate); - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); /* Get info about foreign table. */ table = GetForeignTable(rte->relid); @@ -2633,7 +2631,6 @@ postgresBeginDirectModify(ForeignScanState *node, int eflags) EState *estate = node->ss.ps.state; PgFdwDirectModifyState *dmstate; Index rtindex; - RangeTblEntry *rte; Oid userid; ForeignTable *table; UserMapping *user; @@ -2655,11 +2652,10 @@ postgresBeginDirectModify(ForeignScanState *node, int eflags) * Identify which user to do the remote access as. This should match what * ExecCheckRTEPerms() does. */ - rtindex = node->resultRelInfo->ri_RangeTableIndex; - rte = exec_rt_fetch(rtindex, estate); - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + userid = OidIsValid(fsplan->checkAsUser) ? fsplan->checkAsUser : GetUserId(); /* Get info about foreign table. */ + rtindex = node->resultRelInfo->ri_RangeTableIndex; if (fsplan->scan.scanrelid == 0) dmstate->rel = ExecOpenScanRelation(estate, rtindex, eflags); else @@ -3983,7 +3979,7 @@ create_foreign_modify(EState *estate, * Identify which user to do the remote access as. This should match what * ExecCheckRTEPerms() does. */ - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + userid = OidIsValid(rte->checkAsUser) ? rte->checkAsUser : GetUserId(); /* Get info about foreign table. */ table = GetForeignTable(RelationGetRelid(rel)); diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index e301c687e3..8bf2ba1c04 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -631,7 +631,7 @@ ExecCheckRTEPerms(RangeTblEntry *rte) * call it once in ExecCheckRTPerms and pass the userid down from there. * But for now, no need for the extra clutter. */ - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + userid = OidIsValid(rte->checkAsUser) ? rte->checkAsUser : GetUserId(); /* * We must have *all* the requiredPerms bits, but some of the bits can be diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index ac86ce9003..5013ac3377 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -4148,6 +4148,9 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path, /* Copy cost data from Path to Plan; no need to make FDW do this */ copy_generic_path_info(&scan_plan->scan.plan, &best_path->path); + /* Copy user OID to access as; likewise no need to make FDW do this */ + scan_plan->checkAsUser = rel->userid; + /* Copy foreign server OID; likewise, no need to make FDW do this */ scan_plan->fs_server = rel->serverid; @@ -5794,7 +5797,8 @@ make_foreignscan(List *qptlist, node->operation = CMD_SELECT; node->resultRelation = 0; - /* fs_server will be filled in by create_foreignscan_plan */ + /* checkAsUser, fs_server will be filled in by create_foreignscan_plan */ + node->checkAsUser = InvalidOid; node->fs_server = InvalidOid; node->fdw_exprs = fdw_exprs; node->fdw_private = fdw_private; diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c index b2a7237430..f49cfb6cc6 100644 --- a/src/backend/rewrite/rowsecurity.c +++ b/src/backend/rewrite/rowsecurity.c @@ -128,7 +128,7 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index, return; /* Switch to checkAsUser if it's set */ - user_id = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + user_id = OidIsValid(rte->checkAsUser) ? rte->checkAsUser : GetUserId(); /* Determine the state of RLS for this, pass checkAsUser explicitly */ rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false); diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c index ab97e71dd7..c1652cb4c5 100644 --- a/src/backend/statistics/extended_stats.c +++ b/src/backend/statistics/extended_stats.c @@ -1598,6 +1598,7 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid, Bitmapset **attnums, List **exprs) { RangeTblEntry *rte = root->simple_rte_array[relid]; + RelOptInfo *rel = root->simple_rel_array[relid]; RestrictInfo *rinfo; int clause_relid; Oid userid; @@ -1646,10 +1647,9 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid, return false; /* - * Check that the user has permission to read all required attributes. Use - * checkAsUser if it's set, in case we're accessing the table via a view. + * Check that the user has permission to read all required attributes. */ - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + userid = OidIsValid(rel->userid) ? rel->userid : GetUserId(); /* Table-level SELECT privilege is sufficient for all columns */ if (pg_class_aclcheck(rte->relid, userid, ACL_SELECT) != ACLCHECK_OK) diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index f116924d3c..db21cf3c35 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -5155,10 +5155,11 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid, Assert(rte->rtekind == RTE_RELATION); /* - * Use checkAsUser if it's set, in case we're - * accessing the table via a view. + * Use onerel->userid if it's set, in case + * we're accessing the table via a view. */ - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + userid = OidIsValid(onerel->userid) ? + onerel->userid : GetUserId(); /* * For simplicity, we insist on the whole @@ -5210,7 +5211,8 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid, rte = planner_rt_fetch(varno, root); Assert(rte->rtekind == RTE_RELATION); - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + userid = OidIsValid(onerel->userid) ? + onerel->userid : GetUserId(); vardata->acl_ok = rte->securityQuals == NIL && @@ -5290,10 +5292,11 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid, vardata->freefunc = ReleaseDummy; /* - * Use checkAsUser if it's set, in case we're accessing + * Use onerel->userid if it's set, in case we're accessing * the table via a view. */ - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + userid = OidIsValid(onerel->userid) ? + onerel->userid : GetUserId(); /* * For simplicity, we insist on the whole table being @@ -5341,7 +5344,8 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid, rte = planner_rt_fetch(varno, root); Assert(rte->rtekind == RTE_RELATION); - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + userid = OidIsValid(onerel->userid) ? + onerel->userid : GetUserId(); vardata->acl_ok = rte->securityQuals == NIL && @@ -5402,15 +5406,17 @@ examine_simple_variable(PlannerInfo *root, Var *var, if (HeapTupleIsValid(vardata->statsTuple)) { + RelOptInfo *onerel = find_base_rel(root, var->varno); Oid userid; /* * Check if user has permission to read this column. We require * all rows to be accessible, so there must be no securityQuals - * from security barrier views or RLS policies. Use checkAsUser - * if it's set, in case we're accessing the table via a view. + * from security barrier views or RLS policies. Use + * onerel->userid if it's set, in case we're accessing the table + * via a view. */ - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + userid = OidIsValid(onerel->userid) ? onerel->userid : GetUserId(); vardata->acl_ok = rte->securityQuals == NIL && @@ -5479,7 +5485,8 @@ examine_simple_variable(PlannerInfo *root, Var *var, rte = planner_rt_fetch(varno, root); Assert(rte->rtekind == RTE_RELATION); - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + userid = OidIsValid(onerel->userid) ? + onerel->userid : GetUserId(); vardata->acl_ok = rte->securityQuals == NIL && diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c index 75d42c9ec3..58effdb1c9 100644 --- a/src/backend/utils/misc/rls.c +++ b/src/backend/utils/misc/rls.c @@ -51,7 +51,7 @@ int check_enable_rls(Oid relid, Oid checkAsUser, bool noError) { - Oid user_id = checkAsUser ? checkAsUser : GetUserId(); + Oid user_id = OidIsValid(checkAsUser) ? checkAsUser : GetUserId(); HeapTuple tuple; Form_pg_class classform; bool relrowsecurity; diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index a544b313d3..ef95429a0d 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -901,7 +901,7 @@ typedef struct RelOptInfo */ /* identifies server for the table or join */ Oid serverid; - /* identifies user to check access as */ + /* identifies user to check access as; 0 means to check as current user */ Oid userid; /* join is only valid for current user */ bool useridiscurrent; diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 5c2ab1b379..61cae463fb 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -703,6 +703,8 @@ typedef struct ForeignScan Scan scan; CmdType operation; /* SELECT/INSERT/UPDATE/DELETE */ Index resultRelation; /* direct modification target's RT index */ + Oid checkAsUser; /* user to perform the scan as; 0 means to + * check as current user */ Oid fs_server; /* OID of foreign server */ List *fdw_exprs; /* expressions that FDW may evaluate */ List *fdw_private; /* private data for FDW */