On 2022-Dec-12, Amit Langote wrote:
> On Sun, Dec 11, 2022 at 6:25 PM Amit Langote <amitlangot...@gmail.com> wrote:
> > I've attached 0001 to remove those extraneous code blocks and add a
> > comment mentioning that userid need not be recomputed.
> >
> > While staring at the build_simple_rel() bit mentioned above, I
> > realized that this code fails to set userid correctly in the
> > inheritance parent rels that are child relations of subquery parent
> > relations, such as UNION ALL subqueries. In that case, instead of
> > copying the userid (= 0) of the parent rel, the child should look up
> > its own RTEPermissionInfo, which should be there, and use the
> > checkAsUser value from there. I've attached 0002 to fix this hole. I
> > am not sure whether there's a way to add a test case for this in the
> > core suite.
>
> Ah, I realized we could just expand the test added by 553d2ec27 with a
> wrapper view (to test checkAsUser functionality) and a UNION ALL query
> over the view (to test this change).
Hmm, but if I run this test without the code change in 0002, the test
also passes (to wit: the plan still has two hash joins), so I understand
that either you're testing something that's not changed by the patch,
or the test case itself isn't really what you wanted.
As for 0001, it seems simpler to me to put the 'userid' variable in the
same scope as 'onerel', and then compute it just once and don't bother
with it at all afterwards, as in the attached.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".
gpg: Firmado el lun 16 ene 2023 17:23:19 CET
gpg: usando RSA clave E2C96E4A9BCA7E92A8E3DA551C20ACB9D5C564AE
gpg: Firma correcta de "Álvaro Herrera <alvhe...@alvh.no-ip.org>" [absoluta]
gpg: alias "Álvaro Herrera (PostgreSQL Global Development Group) <alvhe...@postgresql.org>" [absoluta]
gpg: alias "Álvaro Herrera (2ndQuadrant) <alvhe...@2ndquadrant.com>" [absoluta]
commit cdc09715305d98457c8240b579528b7835c39d58
Author: Alvaro Herrera <alvhe...@alvh.no-ip.org> [Álvaro Herrera <alvhe...@alvh.no-ip.org>]
AuthorDate: Sun Dec 11 18:12:05 2022 +0900
CommitDate: Mon Jan 16 17:23:19 2023 +0100
Remove some dead code in selfuncs.c
RelOptInfo.userid is the same for all relations in a given
inheritance tree, so the code in examine_variable() and
example_simple_variable() that wants to repeat the ACL checks on
the root parent rel instead of a given leaf child relations need not
recompute userid too.
Author: Amit Langote
Reported-by: Justin Pryzby
Discussion: https://postgr.es/m/20221210201753.ga27...@telsasoft.com
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index 75bc20c7c9..0a5632699d 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -500,7 +500,6 @@ find_join_rel(PlannerInfo *root, Relids relids)
*
* Otherwise these fields are left invalid, so GetForeignJoinPaths will not be
* called for the join relation.
- *
*/
static void
set_foreign_rel_properties(RelOptInfo *joinrel, RelOptInfo *outer_rel,
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 57de51f0db..4e4888dde4 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -5080,6 +5080,18 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
*/
ListCell *ilist;
ListCell *slist;
+ Oid userid;
+
+ /*
+ * Determine the user ID to use for privilege checks: either
+ * onerel->userid if it's set (e.g., in case we're accessing the table
+ * via a view), or the current user otherwise.
+ *
+ * If we drill down to child relations, we keep using the same userid:
+ * it's going to be the same anyway, due to how we set up the relation
+ * tree (q.v. build_simple_rel).
+ */
+ userid = OidIsValid(onerel->userid) ? onerel->userid : GetUserId();
foreach(ilist, onerel->indexlist)
{
@@ -5150,18 +5162,10 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
{
/* Get index's table for permission check */
RangeTblEntry *rte;
- Oid userid;
rte = planner_rt_fetch(index->rel->relid, root);
Assert(rte->rtekind == RTE_RELATION);
- /*
- * Use onerel->userid if it's set, in case
- * we're accessing the table via a view.
- */
- userid = OidIsValid(onerel->userid) ?
- onerel->userid : GetUserId();
-
/*
* For simplicity, we insist on the whole
* table being selectable, rather than trying
@@ -5212,9 +5216,6 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
rte = planner_rt_fetch(varno, root);
Assert(rte->rtekind == RTE_RELATION);
- userid = OidIsValid(onerel->userid) ?
- onerel->userid : GetUserId();
-
vardata->acl_ok =
rte->securityQuals == NIL &&
(pg_class_aclcheck(rte->relid,
@@ -5281,8 +5282,6 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
/* found a match, see if we can extract pg_statistic row */
if (equal(node, expr))
{
- Oid userid;
-
/*
* XXX Not sure if we should cache the tuple somewhere.
* Now we just create a new copy every time.
@@ -5292,13 +5291,6 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
vardata->freefunc = ReleaseDummy;
- /*
- * Use onerel->userid if it's set, in case we're accessing
- * the table via a view.
- */
- userid = OidIsValid(onerel->userid) ?
- onerel->userid : GetUserId();
-
/*
* For simplicity, we insist on the whole table being
* selectable, rather than trying to identify which
@@ -5345,9 +5337,6 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
rte = planner_rt_fetch(varno, root);
Assert(rte->rtekind == RTE_RELATION);
- userid = OidIsValid(onerel->userid) ?
- onerel->userid : GetUserId();
-
vardata->acl_ok =
rte->securityQuals == NIL &&
(pg_class_aclcheck(rte->relid,
@@ -5486,9 +5475,10 @@ examine_simple_variable(PlannerInfo *root, Var *var,
rte = planner_rt_fetch(varno, root);
Assert(rte->rtekind == RTE_RELATION);
- userid = OidIsValid(onerel->userid) ?
- onerel->userid : GetUserId();
-
+ /*
+ * Fine to use the same userid as it's the same in all
+ * relations of a given inheritance tree.
+ */
vardata->acl_ok =
rte->securityQuals == NIL &&
((pg_class_aclcheck(rte->relid, userid,
