Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Wed, Dec 21, 2022 at 01:44:11PM -0600, Justin Pryzby wrote: > Alvaro could you comment on this ? I added here so it's not forgotten. https://commitfest.postgresql.org/42/4107/
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
Justin Pryzby writes: > On Wed, Dec 21, 2022 at 01:44:11PM -0600, Justin Pryzby wrote: >> Alvaro could you comment on this ? I believe Alvaro's on vacation for a few days more. regards, tom lane
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On 2022-Dec-12, Amit Langote wrote:
> On Sun, Dec 11, 2022 at 6:25 PM Amit Langote 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 " [absoluta]
gpg: alias "Álvaro Herrera (PostgreSQL Global Development Group) " [absoluta]
gpg: alias "Álvaro Herrera (2ndQuadrant) " [absoluta]
commit cdc09715305d98457c8240b579528b7835c39d58
Author: Alvaro Herrera [Álvaro Herrera ]
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_aclc
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Tue, Jan 17, 2023 at 7:33 PM Alvaro Herrera wrote: > On 2022-Dec-12, Amit Langote wrote: > > On Sun, Dec 11, 2022 at 6:25 PM Amit Langote > > 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. Yeah, the test case is bogus. :-(. It seems that, with the test as written, it's not the partitioned table referenced in the view's query that becomes a child of the UNION ALL parent subquery, but the subquery itself. The bug being fixed in 0002 doesn't affect the planning of this query at all, because child subquery is planned independently of the main query involving UNION ALL because of it being unable to be pushed up into the latter. We want the partitioned table referenced in the child subquery to become a child of the UNION ALL parent subquery for the bug to be relevant. I tried rewriting the test such that the view's subquery does get pulled up such that the partitioned table becomes a child of the UNION ALL subquery. By attaching a debugger, I do see the bug affecting the planning of this query, but still not in a way that changes the plan. I will keep trying but in the meantime I'm attaching 0001 to show the rewritten query and the plan. > 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. That sounds better. Attached as 0002. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v3-0001-Add-test-case-to-test-a-bug-of-build_simple_rel.patch Description: Binary data v3-0002-Remove-some-dead-code-in-selfuncs.c.patch Description: Binary data v3-0003-Correctly-set-userid-of-subquery-rel-s-child-rels.patch Description: Binary data
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On 2023-Jan-19, Amit Langote wrote: > It seems that, with the test as written, it's not the partitioned > table referenced in the view's query that becomes a child of the UNION > ALL parent subquery, but the subquery itself. The bug being fixed in > 0002 doesn't affect the planning of this query at all, because child > subquery is planned independently of the main query involving UNION > ALL because of it being unable to be pushed up into the latter. We > want the partitioned table referenced in the child subquery to become > a child of the UNION ALL parent subquery for the bug to be relevant. > > I tried rewriting the test such that the view's subquery does get > pulled up such that the partitioned table becomes a child of the UNION > ALL subquery. By attaching a debugger, I do see the bug affecting the > planning of this query, but still not in a way that changes the plan. > I will keep trying but in the meantime I'm attaching 0001 to show the > rewritten query and the plan. Thanks for spending time tracking down a test case. I'll try to have a look later today. > > 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. > > That sounds better. Attached as 0002. Pushed this one, thank you. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On 2022-Dec-11, Amit Langote wrote:
> 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.
I gave this a look and I thought it was clearer to have the new
condition depend on rel->reloptkind instead parent or no.
I tried a few things for a new test case, but I was unable to find
anything useful. Maybe an intermediate view, I thought; no dice.
Maybe one with a security barrier would do? Anyway, for now I just kept
what you added in v2.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
>From 423416f41247d5e4fa2e99de411ffa3b5e09cc8e Mon Sep 17 00:00:00 2001
From: amitlan
Date: Wed, 18 Jan 2023 16:49:49 +0900
Subject: [PATCH v3] Correctly set userid of subquery rel's child rels
For a RTE_SUBQUERY parent baserel's child relation that itself is a
RTE_RELATION rel, build_simple_rel() should explicitly look up the
latter's RTEPermissionInfo instead of copying the parent rel's
userid, which would be 0 given that it's a subquery rel.
---
src/backend/optimizer/util/relnode.c | 18 ++---
src/test/regress/expected/inherit.out | 39 +--
src/test/regress/sql/inherit.sql | 28 +--
3 files changed, 77 insertions(+), 8 deletions(-)
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index a70a16238a..6c4550b90f 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -233,12 +233,22 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
rel->serverid = InvalidOid;
if (rte->rtekind == RTE_RELATION)
{
+ Assert(parent == NULL ||
+ parent->rtekind == RTE_RELATION ||
+ parent->rtekind == RTE_SUBQUERY);
+
/*
- * Get the userid from the relation's RTEPermissionInfo, though only
- * the tables mentioned in query are assigned RTEPermissionInfos.
- * Child relations (otherrels) simply use the parent's value.
+ * For any RELATION rte, we need a userid with which to check
+ * permission access. Baserels simply use their own RTEPermissionInfo's
+ * checkAsUser.
+ *
+ * For otherrels normally there's no RTEPermissionInfo, so we use the
+ * parent's, which normally has one. The exceptional case is that the
+ * parent is a subquery, in which case the otherrel will have its own.
*/
- if (parent == NULL)
+ if (rel->reloptkind == RELOPT_BASEREL ||
+ (rel->reloptkind == RELOPT_OTHER_MEMBER_REL &&
+ parent->rtekind == RTE_SUBQUERY))
{
RTEPermissionInfo *perminfo;
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 2d49e765de..143271cd62 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2512,9 +2512,44 @@ explain (costs off)
(6 rows)
reset session authorization;
-revoke all on permtest_parent from regress_no_child_access;
-drop role regress_no_child_access;
+-- Check with a view over permtest_parent and a UNION ALL over the view,
+-- especially that permtest_parent's permissions are checked with the role
+-- owning the view as permtest_parent's RTE's checkAsUser.
+create role regress_permtest_view_owner;
+revoke all on permtest_grandchild from regress_permtest_view_owner;
+grant select(a, c) on permtest_parent to regress_permtest_view_owner;
+create view permtest_parent_view as
+ select a, c from permtest_parent;
+alter view permtest_parent_view owner to regress_permtest_view_owner;
+-- Like above, the 2nd arm of UNION ALL gets a hash join due to lack of access
+-- to the expression index's stats
+revoke select on permtest_parent_view from regress_no_child_access;
+grant select(a,c) on permtest_parent_view to regress_no_child_access;
+set session authorization regress_no_child_access;
+explain (costs off)
+ select p1.a, p2.c from
+ (select * from permtest_parent_view
+ union all
+ select * from permtest_parent_view) p1
+ inner join permtest_parent p2 on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
+ QUERY PLAN
+-
+ Hash Join
+ Hash Cond: (p2.a = permtest_parent.a)
+ -> Seq Scan on permtest_grandchild p2
+ -> Hash
+ -> Append
+ -> Seq Scan on permtest_grandchild permtest_parent
+ Filter: ("left"(c, 3) ~ 'a1$'::text)
+ -> Seq Scan on permtest_grandchild permtest_parent_1
+ Filter: ("left"(c, 3) ~ 'a1$'::text)
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On 2023-Feb-17, Alvaro Herrera wrote: > I tried a few things for a new test case, but I was unable to find > anything useful. Maybe an intermediate view, I thought; no dice. > Maybe one with a security barrier would do? Anyway, for now I just kept > what you added in v2. Sorry, I failed to keep count of the patch version correctly. The test case here is what you sent in v3 [1], and consequently the patch I just attached should have been labelled v4. [1] https://postgr.es/m/CA+HiwqF6ricH7HFCkyrK72c=KN-PRkdncxdLmU_mEQx=dra...@mail.gmail.com -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "La vida es para el que se aventura"
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Fri, Feb 17, 2023 at 9:02 PM Alvaro Herrera wrote: > On 2022-Dec-11, Amit Langote wrote: > > 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. > > I gave this a look and I thought it was clearer to have the new > condition depend on rel->reloptkind instead parent or no. Thanks for looking into this again. I agree the condition with reloptkind might be better. > I tried a few things for a new test case, but I was unable to find > anything useful. Maybe an intermediate view, I thought; no dice. > Maybe one with a security barrier would do? Anyway, for now I just kept > what you added in v2. Hmm, I'm fine with leaving the test case out if it doesn't really test the code we're changing, as you also pointed out? One more thing we could try is come up with a postgres_fdw test case, because it uses the RelOptInfo.userid value for remote-costs-based path size estimation. But adding a test case to contrib module's suite test a core planner change might seem strange, ;-). Attaching v4 without the test case. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v4-0001-Correctly-set-userid-of-subquery-rel-s-child-rels.patch Description: Binary data
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On 2023-Feb-20, Amit Langote wrote: > On Fri, Feb 17, 2023 at 9:02 PM Alvaro Herrera > wrote: > > I tried a few things for a new test case, but I was unable to find > > anything useful. Maybe an intermediate view, I thought; no dice. > > Maybe one with a security barrier would do? Anyway, for now I just kept > > what you added in v2. > > Hmm, I'm fine with leaving the test case out if it doesn't really test > the code we're changing, as you also pointed out? Yeah, pushed like that. > One more thing we could try is come up with a postgres_fdw test case, > because it uses the RelOptInfo.userid value for remote-costs-based > path size estimation. But adding a test case to contrib module's > suite test a core planner change might seem strange, ;-). Maybe. Perhaps adding it in a separate file there is okay? -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Small aircraft do not crash frequently ... usually only once!" (ponder, http://thedailywtf.com/)
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
Alvaro Herrera writes: > On 2023-Feb-20, Amit Langote wrote: >> One more thing we could try is come up with a postgres_fdw test case, >> because it uses the RelOptInfo.userid value for remote-costs-based >> path size estimation. But adding a test case to contrib module's >> suite test a core planner change might seem strange, ;-). > Maybe. Perhaps adding it in a separate file there is okay? There is plenty of stuff in contrib module tests that is really there to test core-code behavior. (You could indeed argue that *all* of contrib is there for that purpose.) If it's not convenient to test something without an extension, just do it and don't sweat about that. regards, tom lane
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Tue, Feb 21, 2023 at 12:40 AM Tom Lane wrote: > Alvaro Herrera writes: > > On 2023-Feb-20, Amit Langote wrote: > >> One more thing we could try is come up with a postgres_fdw test case, > >> because it uses the RelOptInfo.userid value for remote-costs-based > >> path size estimation. But adding a test case to contrib module's > >> suite test a core planner change might seem strange, ;-). > > > Maybe. Perhaps adding it in a separate file there is okay? > > There is plenty of stuff in contrib module tests that is really > there to test core-code behavior. (You could indeed argue that > *all* of contrib is there for that purpose.) If it's not > convenient to test something without an extension, just do it > and don't sweat about that. OK. Attached adds a test case to postgres_fdw's suite. You can see that it fails without a316a3bc. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v1-0001-postgres_fdw-test-userid-propagation-to-rels-unde.patch Description: Binary data
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote:
> 0002 contains changes that has to do with changing how we access
> checkAsUser in some foreign table planning/execution code sites.
> Thought it might be better to describe it separately too.
This was committed as 599b33b94:
Stop accessing checkAsUser via RTE in some cases
Which does this in a couple places in selfuncs.c:
if (!vardata->acl_ok &&
root->append_rel_array != NULL)
{
AppendRelInfo *appinfo;
Index varno = index->rel->relid;
appinfo = root->append_rel_array[varno];
while (appinfo &&
planner_rt_fetch(appinfo->parent_relid,
root)->rtekind ==
RTE_RELATION)
{
varno = appinfo->parent_relid;
appinfo = root->append_rel_array[varno];
}
if (varno != index->rel->relid)
{
/* Repeat access check on this rel */
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 &&
(pg_class_aclcheck(rte->relid,
userid,
ACL_SELECT) ==
ACLCHECK_OK);
}
}
The original code rechecks rte->checkAsUser with the rte of the parent
rel. The patch changed to access onerel instead, but that's not updated
after looping to find the parent.
Is that okay ? It doesn't seem intentional, since "userid" is still
being recomputed, but based on onerel, which hasn't changed. The
original intent (since 553d2ec27) is to recheck the parent's
"checkAsUser".
It seems like this would matter for partitioned tables, when the
partition isn't readable, but its parent is, and accessed via a view
owned by another user?
--
Justin
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
Hi,
On Sun, Dec 11, 2022 at 5:17 AM Justin Pryzby wrote:
> On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote:
> > 0002 contains changes that has to do with changing how we access
> > checkAsUser in some foreign table planning/execution code sites.
> > Thought it might be better to describe it separately too.
>
> This was committed as 599b33b94:
> Stop accessing checkAsUser via RTE in some cases
>
> Which does this in a couple places in selfuncs.c:
>
> if (!vardata->acl_ok &&
> root->append_rel_array != NULL)
> {
> AppendRelInfo *appinfo;
> Index varno = index->rel->relid;
>
> appinfo = root->append_rel_array[varno];
> while (appinfo &&
>
> planner_rt_fetch(appinfo->parent_relid,
> root)->rtekind ==
> RTE_RELATION)
> {
> varno = appinfo->parent_relid;
> appinfo =
> root->append_rel_array[varno];
> }
> if (varno != index->rel->relid)
> {
> /* Repeat access check on this rel */
> 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 &&
> (pg_class_aclcheck(rte->relid,
>userid,
>ACL_SELECT) ==
> ACLCHECK_OK);
> }
> }
>
>
> The original code rechecks rte->checkAsUser with the rte of the parent
> rel. The patch changed to access onerel instead, but that's not updated
> after looping to find the parent.
>
> Is that okay ? It doesn't seem intentional, since "userid" is still
> being recomputed, but based on onerel, which hasn't changed. The
> original intent (since 553d2ec27) is to recheck the parent's
> "checkAsUser".
>
> It seems like this would matter for partitioned tables, when the
> partition isn't readable, but its parent is, and accessed via a view
> owned by another user?
Thanks for pointing this out.
I think these blocks which are rewriting userid to basically the same
value should have been removed from these sites as part of 599b33b94.
Even before that commit, the checkAsUser value should have been the
same in the RTE of both the child relation passed to these functions
and that of the root parent that's looked up by looping. That's
because expand_single_inheritance_child(), which adds child RTEs,
copies the parent RTE's checkAsUser, that is, as of commit 599b33b94.
A later commit a61b1f74823c has removed the checkAsUser field from
RangeTblEntry.
Moreover, 599b33b94 adds some code in build_simple_rel() to set a
given rel's userid value by copying it from the parent rel, such that
the userid value would be the same in all relations in a given
inheritance tree.
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.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
v1-0001-Remove-some-dead-code-in-selfuncs.c.patch
Description: Binary data
v1-0002-Fix-build_simple_rel-to-correctly-set-childrel-us.patch
Description: Binary data
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Sun, Dec 11, 2022 at 06:25:48PM +0900, Amit Langote wrote: > On Sun, Dec 11, 2022 at 5:17 AM Justin Pryzby wrote: > > The original code rechecks rte->checkAsUser with the rte of the parent > > rel. The patch changed to access onerel instead, but that's not updated > > after looping to find the parent. > > > > Is that okay ? It doesn't seem intentional, since "userid" is still > > being recomputed, but based on onerel, which hasn't changed. The > > original intent (since 553d2ec27) is to recheck the parent's > > "checkAsUser". > > > > It seems like this would matter for partitioned tables, when the > > partition isn't readable, but its parent is, and accessed via a view > > owned by another user? > > Thanks for pointing this out. > > I think these blocks which are rewriting userid to basically the same > value should have been removed from these sites as part of 599b33b94. I thought maybe; thanks for checking. Little nitpicks: 001: Fine to use the same userid as it's same in all => the same 002: give that it's a subquery rel. => given -- Justin
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Sun, Dec 11, 2022 at 6:25 PM Amit Langote 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). I've done that in the attached updated patch, in which I've also addressed Justin's comments. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v2-0001-Remove-some-dead-code-in-selfuncs.c.patch Description: Binary data v2-0002-Correctly-set-userid-of-subquery-rel-s-child-rels.patch Description: Binary data
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
Alvaro could you comment on this ?
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
Hi, On Tue, Feb 21, 2023 at 4:12 PM Amit Langote wrote: > On Tue, Feb 21, 2023 at 12:40 AM Tom Lane wrote: > > Alvaro Herrera writes: > > > On 2023-Feb-20, Amit Langote wrote: > > >> One more thing we could try is come up with a postgres_fdw test case, > > >> because it uses the RelOptInfo.userid value for remote-costs-based > > >> path size estimation. But adding a test case to contrib module's > > >> suite test a core planner change might seem strange, ;-). > > > > > Maybe. Perhaps adding it in a separate file there is okay? > > > > There is plenty of stuff in contrib module tests that is really > > there to test core-code behavior. (You could indeed argue that > > *all* of contrib is there for that purpose.) If it's not > > convenient to test something without an extension, just do it > > and don't sweat about that. > > OK. Attached adds a test case to postgres_fdw's suite. You can see > that it fails without a316a3bc. Noticed that there's an RfC entry for this in the next CF. Here's an updated version of the patch where I updated the comments a bit and the commit message. I'm thinking of pushing this on Friday barring objections. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v2-0001-Add-a-test-case-for-a316a3bc.patch Description: Binary data
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Wed, Jun 28, 2023 at 4:30 PM Amit Langote wrote: > > Hi, > > On Tue, Feb 21, 2023 at 4:12 PM Amit Langote wrote: > > On Tue, Feb 21, 2023 at 12:40 AM Tom Lane wrote: > > > Alvaro Herrera writes: > > > > On 2023-Feb-20, Amit Langote wrote: > > > >> One more thing we could try is come up with a postgres_fdw test case, > > > >> because it uses the RelOptInfo.userid value for remote-costs-based > > > >> path size estimation. But adding a test case to contrib module's > > > >> suite test a core planner change might seem strange, ;-). > > > > > > > Maybe. Perhaps adding it in a separate file there is okay? > > > > > > There is plenty of stuff in contrib module tests that is really > > > there to test core-code behavior. (You could indeed argue that > > > *all* of contrib is there for that purpose.) If it's not > > > convenient to test something without an extension, just do it > > > and don't sweat about that. > > > > OK. Attached adds a test case to postgres_fdw's suite. You can see > > that it fails without a316a3bc. > > Noticed that there's an RfC entry for this in the next CF. Here's an > updated version of the patch where I updated the comments a bit and > the commit message. > > I'm thinking of pushing this on Friday barring objections. Seeing none, I've pushed this to HEAD and 16. Marking the CF entry as committed. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
