Обсуждение: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?
I poked into the odd behavior reported in bug #15251: https://www.postgresql.org/message-id/152967245839.1266.6939666809369185595@wrigleys.postgresql.org The reason for the poor plan chosen when the caller hasn't got select privilege on the child table is that statistic_proc_security_check() decides not to allow use of the stats for the child table. There are two functions for which it decides that, because they aren't leakproof: textregexeq() and btint4cmp(). Now, it's probably necessary that we consider textregexeq() leaky, since it might spit up errors about its pattern argument. But btint4cmp? It seems pretty silly that the integer comparison operators are marked leakproof while their underlying comparison function isn't. (The reason we're hitting this is that calc_arraycontsel() finds the datatype's default btree comparison function and tries to use that to estimate selectivity. While marking btint4cmp leakproof doesn't completely fix the misestimation, it goes a long way in this example.) I propose to run through the system operator classes, find any for which the comparison function isn't marked leakproof but the operators are, and fix them. This is clearly appropriate for HEAD and maybe it's not too late to force an initdb for v11 --- thoughts? Another question that could be raised is why we are refusing to use stats for a child table when the caller has select on the parent. It's completely trivial to extract data from a child table if you have select on the parent, so it seems like we are checking the wrong table's privileges. regards, tom lane
On 2018-Jul-10, Tom Lane wrote: > I propose to run through the system operator classes, find any for which > the comparison function isn't marked leakproof but the operators are, > and fix them. This is clearly appropriate for HEAD and maybe it's not > too late to force an initdb for v11 --- thoughts? on initdb in v11, see https://postgr.es/m/CAKJS1f9cqoSKS9JVcBKGa2mdn-24YPWc9XLzFDNsmjJMUpth1w@mail.gmail.com -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes: > on initdb in v11, see > https://postgr.es/m/CAKJS1f9cqoSKS9JVcBKGa2mdn-24YPWc9XLzFDNsmjJMUpth1w@mail.gmail.com [confused...] I recall a thread mentioning that we might need initdb in v11, but that one doesn't seem to be it? regards, tom lane
I wrote:
> I propose to run through the system operator classes, find any for which
> the comparison function isn't marked leakproof but the operators are,
> and fix them. This is clearly appropriate for HEAD and maybe it's not
> too late to force an initdb for v11 --- thoughts?
I did that for the built-in btree opclasses. I decided that it's probably
not worth forcing an initdb in v11 for, though. In principle, losing
selectivity estimates because of non-leakproof functions should only
happen in queries that are going to fail at runtime anyway. The real
problem that ought to be addressed and perhaps back-patched is this:
> Another question that could be raised is why we are refusing to use
> stats for a child table when the caller has select on the parent.
> It's completely trivial to extract data from a child table if you
> have select on the parent, so it seems like we are checking the
> wrong table's privileges.
regards, tom lane
On 12.07.18 00:52, Tom Lane wrote: >> Another question that could be raised is why we are refusing to use >> stats for a child table when the caller has select on the parent. >> It's completely trivial to extract data from a child table if you >> have select on the parent, so it seems like we are checking the >> wrong table's privileges. That seems like an oversight. The underlying principle is that we want to allow access to statistics if the user could read the table, or more accurately the column, anyway. This could also happen through inheritance, so we should check that as well, but we need to make sure that the particular column is inherited and not added locally. Also, for the expression index case, we don't track the individual columns, so we don't have that information. For partitioning, we can rely on all the columns being inherited, but not for plain inheritance. So there are some details to work through, it seems. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> ... For
> partitioning, we can rely on all the columns being inherited, but not
> for plain inheritance.
Uh, what?
regards, tom lane
On Sat, Jul 14, 2018 at 11:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > > ... For > > partitioning, we can rely on all the columns being inherited, but not > > for plain inheritance. > > Uh, what? Maybe he meant that partitioning doesn't allow locally defined columns in children, but plain inheritance does. Btw, Peter also said this earlier in the paragraph: "This could also happen through inheritance, so we should check that as well, but we need to make sure that the particular column is inherited and not added locally." But maybe for the case under question, that's irrelevant, because we're only interested in access to inherited columns as those are the only ones that can be accessed in queries via parent. Thanks, Amit
Amit Langote <amitlangote09@gmail.com> writes:
> On Sat, Jul 14, 2018 at 11:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>> ... For
>>> partitioning, we can rely on all the columns being inherited, but not
>>> for plain inheritance.
>> Uh, what?
> But maybe for the case under question, that's irrelevant, because
> we're only interested in access to inherited columns as those are the
> only ones that can be accessed in queries via parent.
Yeah, that's what I thought. It seems like it should be possible to base
all stats access decisions off the table actually named in the query,
because only columns appearing in that table could be referenced, and only
that table's permissions actually get checked at runtime.
I guess it's possible that a child table could have, say, an index on
column X (inherited) and column Y (local) and that some aspect of costing
might then be interested in the behavior of column Y, even though the
query could only mention X not Y. But then we could fall back to the
existing behavior.
regards, tom lane
On Mon, Oct 22, 2018 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Langote <amitlangote09@gmail.com> writes: > > On Sat, Jul 14, 2018 at 11:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > >>> ... For > >>> partitioning, we can rely on all the columns being inherited, but not > >>> for plain inheritance. > > >> Uh, what? > > > But maybe for the case under question, that's irrelevant, because > > we're only interested in access to inherited columns as those are the > > only ones that can be accessed in queries via parent. > > Yeah, that's what I thought. It seems like it should be possible to base > all stats access decisions off the table actually named in the query, > because only columns appearing in that table could be referenced, and only > that table's permissions actually get checked at runtime. > > I guess it's possible that a child table could have, say, an index on > column X (inherited) and column Y (local) and that some aspect of costing > might then be interested in the behavior of column Y, even though the > query could only mention X not Y. But then we could fall back to the > existing behavior. Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can recursively fetch its parent until we reach to the base relation (which is actually named in the query). And, once we have the base relation we can check ACL on that and set vardata->acl_ok accordingly. Additionally, for getting the parent RTI we need to traverse "root->append_rel_list". Another alternative could be that we can add parent_rti member in RelOptInfo structure. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On 2018/10/25 19:54, Dilip Kumar wrote: > On Mon, Oct 22, 2018 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Amit Langote <amitlangote09@gmail.com> writes: >>> But maybe for the case under question, that's irrelevant, because >>> we're only interested in access to inherited columns as those are the >>> only ones that can be accessed in queries via parent. >> >> Yeah, that's what I thought. It seems like it should be possible to base >> all stats access decisions off the table actually named in the query, >> because only columns appearing in that table could be referenced, and only >> that table's permissions actually get checked at runtime. >> >> I guess it's possible that a child table could have, say, an index on >> column X (inherited) and column Y (local) and that some aspect of costing >> might then be interested in the behavior of column Y, even though the >> query could only mention X not Y. But then we could fall back to the >> existing behavior. > > Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can > recursively fetch its parent until we reach to the base relation > (which is actually named in the query). And, once we have the base > relation we can check ACL on that and set vardata->acl_ok accordingly. > Additionally, for getting the parent RTI we need to traverse > "root->append_rel_list". Another alternative could be that we can add > parent_rti member in RelOptInfo structure. Adding parent_rti would be a better idea [1]. I think that traversing append_rel_list every time would be inefficient. Thanks, Amit [1] I've named it inh_root_parent in one of the patches I'm working on where I needed such a field (https://commitfest.postgresql.org/20/1778/)
On Fri, Oct 26, 2018 at 1:12 PM Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > On 2018/10/25 19:54, Dilip Kumar wrote: > > On Mon, Oct 22, 2018 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Amit Langote <amitlangote09@gmail.com> writes: > >>> But maybe for the case under question, that's irrelevant, because > >>> we're only interested in access to inherited columns as those are the > >>> only ones that can be accessed in queries via parent. > >> > >> Yeah, that's what I thought. It seems like it should be possible to base > >> all stats access decisions off the table actually named in the query, > >> because only columns appearing in that table could be referenced, and only > >> that table's permissions actually get checked at runtime. > >> > >> I guess it's possible that a child table could have, say, an index on > >> column X (inherited) and column Y (local) and that some aspect of costing > >> might then be interested in the behavior of column Y, even though the > >> query could only mention X not Y. But then we could fall back to the > >> existing behavior. > > > > Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can > > recursively fetch its parent until we reach to the base relation > > (which is actually named in the query). And, once we have the base > > relation we can check ACL on that and set vardata->acl_ok accordingly. > > Additionally, for getting the parent RTI we need to traverse > > "root->append_rel_list". Another alternative could be that we can add > > parent_rti member in RelOptInfo structure. > > Adding parent_rti would be a better idea [1]. I think that traversing > append_rel_list every time would be inefficient. > [1] I've named it inh_root_parent in one of the patches I'm working on > where I needed such a field (https://commitfest.postgresql.org/20/1778/) Ok, Make sense. I have written a patch by adding this variable. There is still one FIXME in the patch, basically, after getting the baserel rte I need to convert child varattno to parent varattno because in case of inheritance that can be different. Do we already have any mapping from child attno to parent attno or we have to look up the cache. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Вложения
On Sat, Oct 27, 2018 at 10:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, Oct 26, 2018 at 1:12 PM Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > > > On 2018/10/25 19:54, Dilip Kumar wrote: > > > On Mon, Oct 22, 2018 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > >> Amit Langote <amitlangote09@gmail.com> writes: > > >>> But maybe for the case under question, that's irrelevant, because > > >>> we're only interested in access to inherited columns as those are the > > >>> only ones that can be accessed in queries via parent. > > >> > > >> Yeah, that's what I thought. It seems like it should be possible to base > > >> all stats access decisions off the table actually named in the query, > > >> because only columns appearing in that table could be referenced, and only > > >> that table's permissions actually get checked at runtime. > > >> > > >> I guess it's possible that a child table could have, say, an index on > > >> column X (inherited) and column Y (local) and that some aspect of costing > > >> might then be interested in the behavior of column Y, even though the > > >> query could only mention X not Y. But then we could fall back to the > > >> existing behavior. > > > > > > Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can > > > recursively fetch its parent until we reach to the base relation > > > (which is actually named in the query). And, once we have the base > > > relation we can check ACL on that and set vardata->acl_ok accordingly. > > > Additionally, for getting the parent RTI we need to traverse > > > "root->append_rel_list". Another alternative could be that we can add > > > parent_rti member in RelOptInfo structure. > > > > Adding parent_rti would be a better idea [1]. I think that traversing > > append_rel_list every time would be inefficient. > > [1] I've named it inh_root_parent in one of the patches I'm working on > > where I needed such a field (https://commitfest.postgresql.org/20/1778/) > Ok, Make sense. I have written a patch by adding this variable. > There is still one FIXME in the patch, basically, after getting the > baserel rte I need to convert child varattno to parent varattno > because in case of inheritance that can be different. Do we already > have any mapping from child attno to parent attno or we have to look > up the cache. Attached patch handles this issue. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Вложения
Thank you for creating the patch. On 2018/10/28 20:35, Dilip Kumar wrote: > On Sat, Oct 27, 2018 at 10:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: >> On Fri, Oct 26, 2018 at 1:12 PM Amit Langote wrote: >>> On 2018/10/25 19:54, Dilip Kumar wrote: >>>> Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can >>>> recursively fetch its parent until we reach to the base relation >>>> (which is actually named in the query). And, once we have the base >>>> relation we can check ACL on that and set vardata->acl_ok accordingly. >>>> Additionally, for getting the parent RTI we need to traverse >>>> "root->append_rel_list". Another alternative could be that we can add >>>> parent_rti member in RelOptInfo structure. >>> >>> Adding parent_rti would be a better idea [1]. I think that traversing >>> append_rel_list every time would be inefficient. >>> >>> [1] I've named it inh_root_parent in one of the patches I'm working on >>> where I needed such a field (https://commitfest.postgresql.org/20/1778/) >>> >> Ok, Make sense. I have written a patch by adding this variable. >> There is still one FIXME in the patch, basically, after getting the >> baserel rte I need to convert child varattno to parent varattno >> because in case of inheritance that can be different. Do we already >> have any mapping from child attno to parent attno or we have to look >> up the cache. Sorry I forgot to cc you, but I'd posted a patch on the "speeding up planning with partitions" thread, that's extracted from the bigger patch, which adds inh_root_parent member to RelOptInfo [1]. Find it attached with this email. One of the differences from your patch is that it makes inh_root_parent work not just for partitioning, but to inheritance in general. Also, it codes the changes to build_simple_rel to set inh_root_parent's value a bit differently than your patch. > Attached patch handles this issue. I noticed a typo in your patch: transalate_varattno -> translate_varattno +static int +transalate_varattno(Oid oldrelid, Oid newrelid, int old_attno) +{ + Relation oldrelation = heap_open(oldrelid, NoLock); It doesn't seem nice that it performs a heap_open on the parent relation. + att = TupleDescAttr(old_tupdesc, old_attno - 1); + attname = NameStr(att->attname); + + newtup = SearchSysCacheAttName(newrelid, attname); + if (!newtup) + { + heap_close(oldrelation, NoLock); + return InvalidAttrNumber; + } and + varattno = transalate_varattno(relid, rte->relid, var->varattno); + if (AttributeNumberIsValid(varattno)) + relid = rte->relid; + else + varattno = var->varattno; It's not possible for varattno to be invalid here, because the query on inheritance parent only allows to select parent's columns, so we'd error out before getting here if a column not present in the parent were selected. Anyway, why don't we just use the child table's AppendRelInfo to get the parent's version of varattno instead of creating a new function? It can be done as shown in the attached revised version of the portion of the patch changing selfuncs.c. Please take a look. [1] https://www.postgresql.org/message-id/f06a398a-40a9-efb4-fab9-784400fecf13%40lab.ntt.co.jp
Вложения
On Mon, Oct 29, 2018 at 2:53 PM Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > Thank you for creating the patch. > > On 2018/10/28 20:35, Dilip Kumar wrote: > > On Sat, Oct 27, 2018 at 10:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > >> On Fri, Oct 26, 2018 at 1:12 PM Amit Langote wrote: > >>> On 2018/10/25 19:54, Dilip Kumar wrote: > >>>> Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can > >>>> recursively fetch its parent until we reach to the base relation > >>>> (which is actually named in the query). And, once we have the base > >>>> relation we can check ACL on that and set vardata->acl_ok accordingly. > >>>> Additionally, for getting the parent RTI we need to traverse > >>>> "root->append_rel_list". Another alternative could be that we can add > >>>> parent_rti member in RelOptInfo structure. > >>> > >>> Adding parent_rti would be a better idea [1]. I think that traversing > >>> append_rel_list every time would be inefficient. > >>> > >>> [1] I've named it inh_root_parent in one of the patches I'm working on > >>> where I needed such a field (https://commitfest.postgresql.org/20/1778/) > >>> > >> Ok, Make sense. I have written a patch by adding this variable. > >> There is still one FIXME in the patch, basically, after getting the > >> baserel rte I need to convert child varattno to parent varattno > >> because in case of inheritance that can be different. Do we already > >> have any mapping from child attno to parent attno or we have to look > >> up the cache. > > Sorry I forgot to cc you, but I'd posted a patch on the "speeding up > planning with partitions" thread, that's extracted from the bigger patch, > which adds inh_root_parent member to RelOptInfo [1]. Find it attached > with this email. > > One of the differences from your patch is that it makes inh_root_parent > work not just for partitioning, but to inheritance in general. Also, it > codes the changes to build_simple_rel to set inh_root_parent's value a bit > differently than your patch. > > > Attached patch handles this issue. > > I noticed a typo in your patch: > > transalate_varattno -> translate_varattno > > +static int > +transalate_varattno(Oid oldrelid, Oid newrelid, int old_attno) > +{ > + Relation oldrelation = heap_open(oldrelid, NoLock); > > It doesn't seem nice that it performs a heap_open on the parent relation. > > + att = TupleDescAttr(old_tupdesc, old_attno - 1); > + attname = NameStr(att->attname); > + > + newtup = SearchSysCacheAttName(newrelid, attname); > + if (!newtup) > + { > + heap_close(oldrelation, NoLock); > + return InvalidAttrNumber; > + } > > and > > + varattno = transalate_varattno(relid, rte->relid, var->varattno); > + if (AttributeNumberIsValid(varattno)) > + relid = rte->relid; > + else > + varattno = var->varattno; > > It's not possible for varattno to be invalid here, because the query on > inheritance parent only allows to select parent's columns, so we'd error > out before getting here if a column not present in the parent were selected. Make sense to me. > > > Anyway, why don't we just use the child table's AppendRelInfo to get the > parent's version of varattno instead of creating a new function? It can > be done as shown in the attached revised version of the portion of the > patch changing selfuncs.c. Please take a look. +1 > > [1] > https://www.postgresql.org/message-id/f06a398a-40a9-efb4-fab9-784400fecf13%40lab.ntt.co.jp -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On 2018/11/01 20:34, Dilip Kumar wrote: > On Mon, Oct 29, 2018 at 2:53 PM Amit Langote wrote: >> Anyway, why don't we just use the child table's AppendRelInfo to get the >> parent's version of varattno instead of creating a new function? It can >> be done as shown in the attached revised version of the portion of the >> patch changing selfuncs.c. Please take a look. > > +1 Okay, here are two patches: 0001 adds a new RelOptInfo member inh_root_parent that's set for inheritance child otherrels and contains the RT index of the inheritance parent table mentioned in the query from which they originated. 0002 is your patch that modifies examine_variable, etc. to use the permissions granted on parent before reading stats on otherrel inheritance child tables. I've added your name as the author in the 2nd patch. Thanks, Amit
Вложения
On Fri, Nov 2, 2018 at 1:34 PM Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > On 2018/11/01 20:34, Dilip Kumar wrote: > > On Mon, Oct 29, 2018 at 2:53 PM Amit Langote wrote: > >> Anyway, why don't we just use the child table's AppendRelInfo to get the > >> parent's version of varattno instead of creating a new function? It can > >> be done as shown in the attached revised version of the portion of the > >> patch changing selfuncs.c. Please take a look. > > > > +1 > > Okay, here are two patches: > > 0001 adds a new RelOptInfo member inh_root_parent that's set for > inheritance child otherrels and contains the RT index of the inheritance > parent table mentioned in the query from which they originated. > > 0002 is your patch that modifies examine_variable, etc. to use the > permissions granted on parent before reading stats on otherrel inheritance > child tables. I've added your name as the author in the 2nd patch. > I have looked into the patches and these look fine to me. I have also added it to the next commitfest. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Wed, Jul 10, 2019 at 9:44 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, Nov 2, 2018 at 1:34 PM Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > > > On 2018/11/01 20:34, Dilip Kumar wrote: > > > On Mon, Oct 29, 2018 at 2:53 PM Amit Langote wrote: > > >> Anyway, why don't we just use the child table's AppendRelInfo to get the > > >> parent's version of varattno instead of creating a new function? It can > > >> be done as shown in the attached revised version of the portion of the > > >> patch changing selfuncs.c. Please take a look. > > > > > > +1 > > > > Okay, here are two patches: > > > > 0001 adds a new RelOptInfo member inh_root_parent that's set for > > inheritance child otherrels and contains the RT index of the inheritance > > parent table mentioned in the query from which they originated. > > > > 0002 is your patch that modifies examine_variable, etc. to use the > > permissions granted on parent before reading stats on otherrel inheritance > > child tables. I've added your name as the author in the 2nd patch. > > > > I have looked into the patches and these look fine to me. I have also > added it to the next commitfest. > Hi Amit, I have reviewed your 0001 patch and I think you have already taken a look on 0002. So should I move it to "Ready for Committer" or you want to review it further? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Hi Dilip, On Wed, Jul 10, 2019 at 1:29 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Wed, Jul 10, 2019 at 9:44 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, Nov 2, 2018 at 1:34 PM Amit Langote wrote: > > > Okay, here are two patches: > > > > > > 0001 adds a new RelOptInfo member inh_root_parent that's set for > > > inheritance child otherrels and contains the RT index of the inheritance > > > parent table mentioned in the query from which they originated. > > > > > > 0002 is your patch that modifies examine_variable, etc. to use the > > > permissions granted on parent before reading stats on otherrel inheritance > > > child tables. I've added your name as the author in the 2nd patch. > > > > > > > I have looked into the patches and these look fine to me. I have also > > added it to the next commitfest. > > > Hi Amit, > > I have reviewed your 0001 patch and I think you have already taken a > look on 0002. So should I move it to "Ready for Committer" or you > want to review it further? Thanks for checking. There has been a lot of churn in the inheritance planning code since my last email on this thread, so I'd like to reconsider. I'm busy this week with some things, so I'll try posting something on next Tuesday. Thanks, Amit
On Wed, Jul 10, 2019 at 10:15 AM Amit Langote <amitlangote09@gmail.com> wrote: > > Hi Dilip, > > On Wed, Jul 10, 2019 at 1:29 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Jul 10, 2019 at 9:44 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > On Fri, Nov 2, 2018 at 1:34 PM Amit Langote wrote: > > > > Okay, here are two patches: > > > > > > > > 0001 adds a new RelOptInfo member inh_root_parent that's set for > > > > inheritance child otherrels and contains the RT index of the inheritance > > > > parent table mentioned in the query from which they originated. > > > > > > > > 0002 is your patch that modifies examine_variable, etc. to use the > > > > permissions granted on parent before reading stats on otherrel inheritance > > > > child tables. I've added your name as the author in the 2nd patch. > > > > > > > > > > I have looked into the patches and these look fine to me. I have also > > > added it to the next commitfest. > > > > > Hi Amit, > > > > I have reviewed your 0001 patch and I think you have already taken a > > look on 0002. So should I move it to "Ready for Committer" or you > > want to review it further? > > Thanks for checking. There has been a lot of churn in the inheritance > planning code since my last email on this thread, so I'd like to > reconsider. I'm busy this week with some things, so I'll try posting > something on next Tuesday. > Sounds good. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Wed, Jul 10, 2019 at 2:43 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Wed, Jul 10, 2019 at 10:15 AM Amit Langote <amitlangote09@gmail.com> wrote: > > Thanks for checking. There has been a lot of churn in the inheritance > > planning code since my last email on this thread, so I'd like to > > reconsider. I'm busy this week with some things, so I'll try posting > > something on next Tuesday. > > > Sounds good. I looked at this today and concluded that the problem and the patches discussed here are fairly isolated from inheritance planning changes committed to PG 12. I've combined the two patches into one. I tried to think up test cases to go with the code changes, but couldn't come up with one. Thanks, Amit
Вложения
On Wed, Jul 17, 2019 at 2:39 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Wed, Jul 10, 2019 at 2:43 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Jul 10, 2019 at 10:15 AM Amit Langote <amitlangote09@gmail.com> wrote: > > > Thanks for checking. There has been a lot of churn in the inheritance > > > planning code since my last email on this thread, so I'd like to > > > reconsider. I'm busy this week with some things, so I'll try posting > > > something on next Tuesday. > > > > > Sounds good. > > I looked at this today and concluded that the problem and the patches > discussed here are fairly isolated from inheritance planning changes > committed to PG 12. > > I've combined the two patches into one. Looks fine to me, moved to ready for committer. I tried to think up test > cases to go with the code changes, but couldn't come up with one. I am also not sure how to test whether we have access to the statistics of the table. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Mon, Jul 29, 2019 at 6:59 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Wed, Jul 17, 2019 at 2:39 PM Amit Langote <amitlangote09@gmail.com> wrote: > > I've combined the two patches into one. > Looks fine to me, moved to ready for committer. Thank you Dilip. Regards, Amit
Amit Langote <amitlangote09@gmail.com> writes:
> [ v2-0001-Use-root-parent-s-permissions-when-read-child-tab.patch ]
I took a quick look through this. I have some cosmetic thoughts and
also a couple of substantive concerns:
* As a rule, patches that add fields at the end of a struct are wrong.
There is almost always some more-appropriate place to put the field
based on its semantics. We don't want our struct layouts to be historical
annals; they should reflect a coherent design. In this case I'd be
inclined to put the new field next to the regular relid field. It
should have a name that's less completely unrelated to relid, too.
* It might make more sense to define the new field as "top parent's relid,
or self if no parent", rather than "... or zero if no parent". Then you
don't need if-tests like this:
+ if (rel->inh_root_parent > 0)
+ rte = planner_rt_fetch(rel->inh_root_parent,
+ root);
+ else
+ rte = planner_rt_fetch(rel->relid, root);
In the places where you actually want the other definition, you could
test for inh_root_parent being different from relid. That's slightly
more complicated than testing for nonzero, but there aren't many such
places so I think getting rid of the other if's is more useful.
* The business about reverse mapping Vars seems quite inefficient, but
what's much worse is that it only accounts for one level of parent.
I'm pretty certain this will give the wrong answer for multiple levels
of partitioning, if the column numbers aren't all the same.
* To fix the above, you probably need to map back one inheritance level
at a time, which suggests that you could just use the AppendRelInfo
parent-rel info and not need any addition to RelOptInfo at all. This
makes the efficiency issue even worse, though. I don't immediately have a
great idea about doing better. Making it faster might require adding more
info to AppendRelInfos, and I'm not quite sure if it's worth the tradeoff.
* I'd be inclined to use an actual test-and-elog not just an Assert
for the no-mapping-found case. For one reason, some compilers are
going to complain about a set-but-not-used variable in non-assert
builds. More importantly, I'm not very convinced that it's impossible
to hit the no-mapping case. The original proposal was to fall back
to current behavior (test the child-table permissions) if we couldn't
match the var to the top parent, and I think that that is still a
sane proposal.
As for how to test, it doesn't seem like it should be that hard to devise
a situation where you'll get different plan shapes depending on whether
the planner has an accurate or default selectivity estimate.
regards, tom lane
On Wed, Sep 4, 2019 at 8:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > [ v2-0001-Use-root-parent-s-permissions-when-read-child-tab.patch ]
>
> I took a quick look through this. I have some cosmetic thoughts and
> also a couple of substantive concerns:
Thanks a lot for reviewing this.
> * As a rule, patches that add fields at the end of a struct are wrong.
> There is almost always some more-appropriate place to put the field
> based on its semantics. We don't want our struct layouts to be historical
> annals; they should reflect a coherent design. In this case I'd be
> inclined to put the new field next to the regular relid field. It
> should have a name that's less completely unrelated to relid, too.
I've renamed the field to inh_root_relid and placed it next to relid.
> * It might make more sense to define the new field as "top parent's relid,
> or self if no parent", rather than "... or zero if no parent". Then you
> don't need if-tests like this:
>
> + if (rel->inh_root_parent > 0)
> + rte = planner_rt_fetch(rel->inh_root_parent,
> + root);
> + else
> + rte = planner_rt_fetch(rel->relid, root);
Agreed, done this way.
> * The business about reverse mapping Vars seems quite inefficient, but
> what's much worse is that it only accounts for one level of parent.
> I'm pretty certain this will give the wrong answer for multiple levels
> of partitioning, if the column numbers aren't all the same.
Indeed. We need to be checking the root parent's permissions, not the
immediate parent's which might be a child itself.
> * To fix the above, you probably need to map back one inheritance level
> at a time, which suggests that you could just use the AppendRelInfo
> parent-rel info and not need any addition to RelOptInfo at all. This
> makes the efficiency issue even worse, though. I don't immediately have a
> great idea about doing better. Making it faster might require adding more
> info to AppendRelInfos, and I'm not quite sure if it's worth the tradeoff.
Hmm, yes. If AppendRelInfos had contained a reverse translation list
that maps Vars of a given child to the root parent's, this patch would
end up being much simpler and not add too much cost to the selectivity
code. However building such a map would not be free and the number of
places where it's useful would be significantly smaller where the
existing parent-to-child translation list is used.
Anyway, I've fixed the above-mentioned oversights in the current code for now.
> * I'd be inclined to use an actual test-and-elog not just an Assert
> for the no-mapping-found case. For one reason, some compilers are
> going to complain about a set-but-not-used variable in non-assert
> builds. More importantly, I'm not very convinced that it's impossible
> to hit the no-mapping case. The original proposal was to fall back
> to current behavior (test the child-table permissions) if we couldn't
> match the var to the top parent, and I think that that is still a
> sane proposal.
OK, I've removed the Assert. For child Vars that can't be translated
to root parent's, permissions are checked with the child relation,
like before.
> As for how to test, it doesn't seem like it should be that hard to devise
> a situation where you'll get different plan shapes depending on whether
> the planner has an accurate or default selectivity estimate.
I managed to find a test case by trial-and-error, but it may be more
convoluted than it has to be.
-- On HEAD
create table permtest_parent (a int, b text, c text) partition by list (a);
create table permtest_child (b text, a int, c text) partition by list (b);
create table permtest_grandchild (c text, b text, a int);
alter table permtest_child attach partition permtest_grandchild for
values in ('a');
alter table permtest_parent attach partition permtest_child for values in (1);
insert into permtest_parent select 1, 'a', i || 'x' || i * 10 from
generate_series(1, 1000) i;
analyze permtest_parent;
explain (costs off) select * from permtest_parent p1 inner join
permtest_parent p2 on p1.a = p2.a and p1.c like '4x5%';
QUERY PLAN
---------------------------------------------------------------------------------
Nested Loop (cost=0.00..47.00 rows=1000 width=28)
Join Filter: (p1.a = p2.a)
-> Seq Scan on permtest_grandchild p1 (cost=0.00..18.50 rows=1 width=14)
Filter: (c ~~ '4x5%'::text)
-> Seq Scan on permtest_grandchild p2 (cost=0.00..16.00 rows=1000 width=14)
(5 rows)
create role regress_no_child_access;
revoke all on permtest_grandchild from regress_no_child_access;
grant all on permtest_parent to regress_no_child_access;
set session authorization regress_no_child_access;
explain (costs off) select * from permtest_parent p1 inner join
permtest_parent p2 on p1.a = p2.a and p1.c like '4x5%';
QUERY PLAN
------------------------------------------------------------------------------------
Hash Join (cost=18.56..93.31 rows=5000 width=28)
Hash Cond: (p2.a = p1.a)
-> Seq Scan on permtest_grandchild p2 (cost=0.00..16.00 rows=1000 width=14)
-> Hash (cost=18.50..18.50 rows=5 width=14)
-> Seq Scan on permtest_grandchild p1 (cost=0.00..18.50
rows=5 width=14)
Filter: (c ~~ '4x5%'::text)
(6 rows)
-- Patched:
set session authorization regress_no_child_access;
explain (costs off) select * from permtest_parent p1 inner join
permtest_parent p2 on p1.a = p2.a and p1.c like '4x5%';
QUERY PLAN
---------------------------------------------------------------------------------
Nested Loop (cost=0.00..47.00 rows=1000 width=28)
Join Filter: (p1.a = p2.a)
-> Seq Scan on permtest_grandchild p1 (cost=0.00..18.50 rows=1 width=14)
Filter: (c ~~ '4x5%'::text)
-> Seq Scan on permtest_grandchild p2 (cost=0.00..16.00 rows=1000 width=14)
(5 rows)
Updated patch attached.
Thanks,
Amit
Вложения
On Thu, Sep 5, 2019 at 2:12 PM Amit Langote <amitlangote09@gmail.com> wrote: Thanks for the patch, I was almost about to press the send button with my patch. But, this looks similar to my version. > > On Wed, Sep 4, 2019 at 8:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Amit Langote <amitlangote09@gmail.com> writes: > > > [ v2-0001-Use-root-parent-s-permissions-when-read-child-tab.patch ] > > > > I took a quick look through this. I have some cosmetic thoughts and > > also a couple of substantive concerns: > > Thanks a lot for reviewing this. > > > * As a rule, patches that add fields at the end of a struct are wrong. > > There is almost always some more-appropriate place to put the field > > based on its semantics. We don't want our struct layouts to be historical > > annals; they should reflect a coherent design. In this case I'd be > > inclined to put the new field next to the regular relid field. It > > should have a name that's less completely unrelated to relid, too. > > I've renamed the field to inh_root_relid and placed it next to relid. > > > * It might make more sense to define the new field as "top parent's relid, > > or self if no parent", rather than "... or zero if no parent". Then you > > don't need if-tests like this: > > > > + if (rel->inh_root_parent > 0) > > + rte = planner_rt_fetch(rel->inh_root_parent, > > + root); > > + else > > + rte = planner_rt_fetch(rel->relid, root); > > Agreed, done this way. > > > * The business about reverse mapping Vars seems quite inefficient, but > > what's much worse is that it only accounts for one level of parent. > > I'm pretty certain this will give the wrong answer for multiple levels > > of partitioning, if the column numbers aren't all the same. > > Indeed. We need to be checking the root parent's permissions, not the > immediate parent's which might be a child itself. > > > * To fix the above, you probably need to map back one inheritance level > > at a time, which suggests that you could just use the AppendRelInfo > > parent-rel info and not need any addition to RelOptInfo at all. This > > makes the efficiency issue even worse, though. I don't immediately have a > > great idea about doing better. Making it faster might require adding more > > info to AppendRelInfos, and I'm not quite sure if it's worth the tradeoff. > > Hmm, yes. If AppendRelInfos had contained a reverse translation list > that maps Vars of a given child to the root parent's, this patch would > end up being much simpler and not add too much cost to the selectivity > code. However building such a map would not be free and the number of > places where it's useful would be significantly smaller where the > existing parent-to-child translation list is used. > > Anyway, I've fixed the above-mentioned oversights in the current code for now. > > > * I'd be inclined to use an actual test-and-elog not just an Assert > > for the no-mapping-found case. For one reason, some compilers are > > going to complain about a set-but-not-used variable in non-assert > > builds. More importantly, I'm not very convinced that it's impossible > > to hit the no-mapping case. The original proposal was to fall back > > to current behavior (test the child-table permissions) if we couldn't > > match the var to the top parent, and I think that that is still a > > sane proposal. > > OK, I've removed the Assert. For child Vars that can't be translated > to root parent's, permissions are checked with the child relation, > like before. Instead of falling back to the child, isn't it make more sense to check the permissions on the parent upto which we could translate (it may not be the root parent)? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Hi Dilip, Thanks for checking. On Thu, Sep 5, 2019 at 6:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Thu, Sep 5, 2019 at 2:12 PM Amit Langote <amitlangote09@gmail.com> wrote: > Thanks for the patch, I was almost about to press the send button with > my patch. But, this looks similar to my version. Good to hear that. > > On Wed, Sep 4, 2019 at 8:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > * I'd be inclined to use an actual test-and-elog not just an Assert > > > for the no-mapping-found case. For one reason, some compilers are > > > going to complain about a set-but-not-used variable in non-assert > > > builds. More importantly, I'm not very convinced that it's impossible > > > to hit the no-mapping case. The original proposal was to fall back > > > to current behavior (test the child-table permissions) if we couldn't > > > match the var to the top parent, and I think that that is still a > > > sane proposal. > > > > OK, I've removed the Assert. For child Vars that can't be translated > > to root parent's, permissions are checked with the child relation, > > like before. > > Instead of falling back to the child, isn't it make more sense to > check the permissions on the parent upto which we could translate (it > may not be the root parent)? Hmm, in that case, the parent up to which we might be able to translate would still be a child and might have different permissions than the table mentioned in the query (what's being called "root" in this context). Would it be worth further complicating this code if that's the case? Thanks, Amit
On Thu, Sep 5, 2019 at 2:48 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Thu, Sep 5, 2019 at 2:12 PM Amit Langote <amitlangote09@gmail.com> wrote: > > Thanks for the patch, I was almost about to press the send button with > my patch. But, this looks similar to my version. > > > > On Wed, Sep 4, 2019 at 8:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Instead of falling back to the child, isn't it make more sense to > check the permissions on the parent upto which we could translate (it > may not be the root parent)? > /* + * For inheritance child relations, we also need to remember + * the root parent. + */ + if (parent->rtekind == RTE_RELATION) + rel->inh_root_relid = parent->inh_root_relid > 0 ? + parent->inh_root_relid : + parent->relid; + else + /* Child relation of flattened UNION ALL subquery. */ + rel->inh_root_relid = relid; With the current changes, parent->inh_root_relid will always be > 0 so (parent->inh_root_relid > 0) condition doesn't make sence. Right? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Sep 5, 2019 at 6:33 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> /*
> + * For inheritance child relations, we also need to remember
> + * the root parent.
> + */
> + if (parent->rtekind == RTE_RELATION)
> + rel->inh_root_relid = parent->inh_root_relid > 0 ?
> + parent->inh_root_relid :
> + parent->relid;
> + else
> + /* Child relation of flattened UNION ALL subquery. */
> + rel->inh_root_relid = relid;
>
> With the current changes, parent->inh_root_relid will always be > 0 so
> (parent->inh_root_relid > 0) condition doesn't make sence. Right?
Oops, you're right. It should be:
if (parent->rtekind == RTE_RELATION)
rel->inh_root_relid = parent->inh_root_relid;
else
rel->inh_root_relid = relid;
Updated patch attached.
Thanks,
Amit
Вложения
On Thu, Sep 5, 2019 at 3:26 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Thu, Sep 5, 2019 at 6:33 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > /* > > + * For inheritance child relations, we also need to remember > > + * the root parent. > > + */ > > + if (parent->rtekind == RTE_RELATION) > > + rel->inh_root_relid = parent->inh_root_relid > 0 ? > > + parent->inh_root_relid : > > + parent->relid; > > + else > > + /* Child relation of flattened UNION ALL subquery. */ > > + rel->inh_root_relid = relid; > > > > With the current changes, parent->inh_root_relid will always be > 0 so > > (parent->inh_root_relid > 0) condition doesn't make sence. Right? > > Oops, you're right. It should be: > > if (parent->rtekind == RTE_RELATION) > rel->inh_root_relid = parent->inh_root_relid; > else > rel->inh_root_relid = relid; > Right! > Updated patch attached. > -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Sep 5, 2019 at 6:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>> Instead of falling back to the child, isn't it make more sense to
>> check the permissions on the parent upto which we could translate (it
>> may not be the root parent)?
> Hmm, in that case, the parent up to which we might be able to
> translate would still be a child and might have different permissions
> than the table mentioned in the query (what's being called "root" in
> this context). Would it be worth further complicating this code if
> that's the case?
I think that checking intermediate levels would be an actively bad idea
--- it would make the behavior too confusing. We should preferably check
the table actually named in the query, or if we can't then check the
table we are using the stats of; nothing else.
Another idea that we should consider, though, is to allow the access if
*either* of those two tables allows it. The main reason that that's
attractive is that it's certain not to break any case that works today.
But also, it would mean that in many practical cases we'd not have to
try to map Vars back up to the original parent, thus avoiding the
performance penalty. (That is, check the target table as we do now,
and only if we find it lacks permissions do we start mapping back.)
regards, tom lane
On Fri, Sep 6, 2019 at 12:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Langote <amitlangote09@gmail.com> writes: > > On Thu, Sep 5, 2019 at 6:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > >> Instead of falling back to the child, isn't it make more sense to > >> check the permissions on the parent upto which we could translate (it > >> may not be the root parent)? > > > Hmm, in that case, the parent up to which we might be able to > > translate would still be a child and might have different permissions > > than the table mentioned in the query (what's being called "root" in > > this context). Would it be worth further complicating this code if > > that's the case? > > I think that checking intermediate levels would be an actively bad idea > --- it would make the behavior too confusing. We should preferably check > the table actually named in the query, or if we can't then check the > table we are using the stats of; nothing else. Agreed. > Another idea that we should consider, though, is to allow the access if > *either* of those two tables allows it. The main reason that that's > attractive is that it's certain not to break any case that works today. > But also, it would mean that in many practical cases we'd not have to > try to map Vars back up to the original parent, thus avoiding the > performance penalty. (That is, check the target table as we do now, > and only if we find it lacks permissions do we start mapping back.) Ah, that sounds like a good idea. Patch updated that way. Thanks, Amit
Вложения
Travis complains that this patch adds a new compile warning. Please fix. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Sep 26, 2019 at 5:15 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Travis complains that this patch adds a new compile warning. Please > fix. Thanks, updated patch attached. Regards, Amit
Вложения
Amit Langote <amitlangote09@gmail.com> writes:
> [ v6-0001-Use-root-parent-s-permissions-when-reading-child-.patch ]
I started to review this, and discovered that the new regression test
passes just fine without applying any of the rest of the patch.
Usually we try to design regression test additions so that they
demonstrate that the new code does something different, so this seems
a bit odd. Can't we set up the test to fail with unpatched code?
Also, the test case contains no expression index, so I can't see how
it'd provide any code coverage for the code added in examine_variable.
The comment for inh_root_relid seems rather inadequate, since it
fails to mention the special case for UNION ALL subqueries.
But do we even need that special case? It looks to me like the
walk-up-to-parent code is defending against such cases by checking
relkind, so maybe we don't need to throw away info for UNION ALL.
In general, if we're going to add inh_root_relid, I'd like its
definition to be as simple and consistent as possible, because
I'm sure there will be other uses for it. If it could be something
like "baserel that this otherrel is a child of", full stop,
I think that'd be good.
I don't especially like the logic in examine_simple_variable,
because it walks back up the AppendRelInfo chain but then proceeds
to use
rte = planner_rt_fetch(rel->inh_root_relid, root);
without any sort of cross-check that it's stopped at that relation
and not some other one. It'd be better to keep track of the top
parent_relid while walking up, and use that. Or else make the
loop stop condition be reaching the matching relid.
regards, tom lane
Thanks for the review. On Thu, Nov 21, 2019 at 6:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Langote <amitlangote09@gmail.com> writes: > > [ v6-0001-Use-root-parent-s-permissions-when-reading-child-.patch ] > > I started to review this, and discovered that the new regression test > passes just fine without applying any of the rest of the patch. > Usually we try to design regression test additions so that they > demonstrate that the new code does something different, so this seems > a bit odd. Can't we set up the test to fail with unpatched code? Hmm, the test case *used to* fail without the code fix back in September. That is no longer the case, in short because text_ge() got marked leakproof since then. Anyway, I have modified the test case such that it now fails without the code fix, but I don't have enough faith that it's robust enough. :( > Also, the test case contains no expression index, so I can't see how > it'd provide any code coverage for the code added in examine_variable. Added a test case involving an expression index, which helped spot a problem with the code added in examine_variable, which fixed too. > The comment for inh_root_relid seems rather inadequate, since it > fails to mention the special case for UNION ALL subqueries. > But do we even need that special case? It looks to me like the > walk-up-to-parent code is defending against such cases by checking > relkind, so maybe we don't need to throw away info for UNION ALL. > In general, if we're going to add inh_root_relid, I'd like its > definition to be as simple and consistent as possible, because > I'm sure there will be other uses for it. If it could be something > like "baserel that this otherrel is a child of", full stop, > I think that'd be good. If inh_root_relid meant that, it would no longer be useful to examine_variable. In examine_variable, we need to map a child table's relid to the relid of its root parent table. If the root parent itself is under a UNION ALL subquery parent, then inh_root_relid of all relations in that ancestry chain would point to the UNION ALL subquery parent, which is not what examine_variable would want to use, because it's really looking for the root "table". In examine_simple_variable however, we need to not just map the child relid to root table relid, but also convert the Var, so we have to traverse the ancestry chain via AppendRelInfos. So, we don't really need inh_root_relid there, because we can calculate that as we're traversing the AppendRelInfo chain. I have expanded the comment for inh_root_relid a bit. > I don't especially like the logic in examine_simple_variable, > because it walks back up the AppendRelInfo chain but then proceeds > to use > rte = planner_rt_fetch(rel->inh_root_relid, root); > without any sort of cross-check that it's stopped at that relation > and not some other one. It'd be better to keep track of the top > parent_relid while walking up, and use that. Or else make the > loop stop condition be reaching the matching relid. I've added an Assert to cross-check that the AppendRelInfo traversal loop stops once it has computed a Var matching inh_root_relid. Attached updated patch. Thanks, Amit
Вложения
Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Nov 21, 2019 at 6:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The comment for inh_root_relid seems rather inadequate, since it
>> fails to mention the special case for UNION ALL subqueries.
>> But do we even need that special case? It looks to me like the
>> walk-up-to-parent code is defending against such cases by checking
>> relkind, so maybe we don't need to throw away info for UNION ALL.
>> In general, if we're going to add inh_root_relid, I'd like its
>> definition to be as simple and consistent as possible, because
>> I'm sure there will be other uses for it. If it could be something
>> like "baserel that this otherrel is a child of", full stop,
>> I think that'd be good.
> If inh_root_relid meant that, it would no longer be useful to
> examine_variable. In examine_variable, we need to map a child table's
> relid to the relid of its root parent table. If the root parent
> itself is under a UNION ALL subquery parent, then inh_root_relid of
> all relations in that ancestry chain would point to the UNION ALL
> subquery parent, which is not what examine_variable would want to use,
> because it's really looking for the root "table".
Hm, I see. Still, the definition seems quite ad-hoc and of uncertain
usefulness to any other use-case. Given that checking permissions for
access to an expression index's stats is a pretty uncommon thing to
be doing, I don't really want to let it drive the definition of a
new RelOptInfo field.
The other reason that I'm on the warpath against this field is that
it makes the patch un-back-patchable, and I'd like to be able to fix
this problem in the back branches.
Given the existence of the append_rel_array array, it's not really
difficult or expensive to use that to chain up to the root parent,
as in the attached simplified patch. We could only use this back
to v11 where append_rel_array was added, but that's still a lot
better than no back-patched fix at all.
I've not studied the test case too closely yet, other than to verify
that it does fail without the code fix :-). Other than that, though,
I think this patch is committable for v11 through HEAD.
regards, tom lane
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 26a2e3b..35dbd72 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -4613,6 +4613,52 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
rte->securityQuals == NIL &&
(pg_class_aclcheck(rte->relid, userid,
ACL_SELECT) == ACLCHECK_OK);
+
+ /*
+ * If the user doesn't have permissions to
+ * access an inheritance child relation, check
+ * the permissions of the table actually
+ * mentioned in the query, since most likely
+ * the user does have that permission. Note
+ * that whole-table select privilege on the
+ * parent doesn't quite guarantee that the
+ * user could read all columns of the child.
+ * But in practice it's unlikely that any
+ * interesting security violation could result
+ * from allowing access to the expression
+ * index's stats, so we allow it anyway. See
+ * similar code in examine_simple_variable()
+ * for additional comments.
+ */
+ 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();
+
+ vardata->acl_ok =
+ rte->securityQuals == NIL &&
+ (pg_class_aclcheck(rte->relid,
+ userid,
+ ACL_SELECT) == ACLCHECK_OK);
+ }
+ }
}
else
{
@@ -4690,6 +4736,88 @@ examine_simple_variable(PlannerInfo *root, Var *var,
ACL_SELECT) == ACLCHECK_OK) ||
(pg_attribute_aclcheck(rte->relid, var->varattno, userid,
ACL_SELECT) == ACLCHECK_OK));
+
+ /*
+ * If the user doesn't have permissions to access an inheritance
+ * child relation or specifically this attribute, check the
+ * permissions of the table/column actually mentioned in the
+ * query, since most likely the user does have that permission
+ * (else the query will fail at runtime), and if the user can read
+ * the column there then he can get the values of the child table
+ * too. To do that, we must find out which of the root parent's
+ * attributes the child relation's attribute corresponds to.
+ */
+ if (!vardata->acl_ok && var->varattno > 0 &&
+ root->append_rel_array != NULL)
+ {
+ AppendRelInfo *appinfo;
+ Index varno = var->varno;
+ int varattno = var->varattno;
+ bool found = false;
+
+ appinfo = root->append_rel_array[varno];
+
+ /*
+ * Partitions are mapped to their immediate parent, not the
+ * root parent, so must be ready to walk up multiple
+ * AppendRelInfos. But stop if we hit a parent that is not
+ * RTE_RELATION --- that's a flattened UNION ALL subquery, not
+ * an inheritance parent.
+ */
+ while (appinfo &&
+ planner_rt_fetch(appinfo->parent_relid,
+ root)->rtekind == RTE_RELATION)
+ {
+ int parent_varattno;
+ ListCell *l;
+
+ parent_varattno = 1;
+ found = false;
+ foreach(l, appinfo->translated_vars)
+ {
+ Var *childvar = lfirst_node(Var, l);
+
+ /* Ignore dropped attributes of the parent. */
+ if (childvar != NULL &&
+ varattno == childvar->varattno)
+ {
+ found = true;
+ break;
+ }
+ parent_varattno++;
+ }
+
+ if (!found)
+ break;
+
+ varno = appinfo->parent_relid;
+ varattno = parent_varattno;
+
+ /* If the parent is itself a child, continue up. */
+ appinfo = root->append_rel_array[varno];
+ }
+
+ /*
+ * In rare cases, the Var may be local to the child table, in
+ * which case, we've got to live with having no access to this
+ * column's stats.
+ */
+ if (!found)
+ return;
+
+ /* Repeat the access check on this parent rel & column */
+ rte = planner_rt_fetch(varno, root);
+ Assert(rte->rtekind == RTE_RELATION);
+
+ userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
+
+ vardata->acl_ok =
+ rte->securityQuals == NIL &&
+ ((pg_class_aclcheck(rte->relid, userid,
+ ACL_SELECT) == ACLCHECK_OK) ||
+ (pg_attribute_aclcheck(rte->relid, varattno, userid,
+ ACL_SELECT) == ACLCHECK_OK));
+ }
}
else
{
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 44d51ed..0943ba4 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2335,3 +2335,62 @@ explain (costs off) select * from range_parted order by a desc,b desc,c desc;
(3 rows)
drop table range_parted;
+-- Test for checking that lack of access permissions for a child table and
+-- hence its statistics doesn't affect plan shapes when the query is on the
+-- parent table
+create table permtest_parent (a int, b text, c text) partition by list (a);
+create table permtest_child (b text, a int, c text) partition by list (b);
+create table permtest_grandchild (c text, b text, a int);
+alter table permtest_child attach partition permtest_grandchild for values in ('a');
+alter table permtest_parent attach partition permtest_child for values in (1);
+create index on permtest_parent (left(c, 3));
+insert into permtest_parent select 1, 'a', left(md5(i::text), 5) from generate_series(0, 100) i;
+analyze permtest_parent;
+explain (costs off) select * from permtest_parent p1 inner join permtest_parent p2 on p1.a = p2.a and p1.c ~ 'a1$';
+ QUERY PLAN
+------------------------------------------
+ Nested Loop
+ Join Filter: (p1.a = p2.a)
+ -> Seq Scan on permtest_grandchild p1
+ Filter: (c ~ 'a1$'::text)
+ -> Seq Scan on permtest_grandchild p2
+(5 rows)
+
+explain (costs off) select * from permtest_parent p1 inner join permtest_parent p2 on p1.a = p2.a and left(p1.c, 3) ~
'a1$';
+ QUERY PLAN
+----------------------------------------------
+ Nested Loop
+ Join Filter: (p1.a = p2.a)
+ -> Seq Scan on permtest_grandchild p1
+ Filter: ("left"(c, 3) ~ 'a1$'::text)
+ -> Seq Scan on permtest_grandchild p2
+(5 rows)
+
+create role regress_no_child_access;
+revoke all on permtest_grandchild from regress_no_child_access;
+grant all on permtest_parent to regress_no_child_access;
+set session authorization regress_no_child_access;
+explain (costs off) select * from permtest_parent p1 inner join permtest_parent p2 on p1.a = p2.a and p1.c ~ 'a1$';
+ QUERY PLAN
+------------------------------------------
+ Nested Loop
+ Join Filter: (p1.a = p2.a)
+ -> Seq Scan on permtest_grandchild p1
+ Filter: (c ~ 'a1$'::text)
+ -> Seq Scan on permtest_grandchild p2
+(5 rows)
+
+explain (costs off) select * from permtest_parent p1 inner join permtest_parent p2 on p1.a = p2.a and left(p1.c, 3) ~
'a1$';
+ QUERY PLAN
+----------------------------------------------
+ Nested Loop
+ Join Filter: (p1.a = p2.a)
+ -> Seq Scan on permtest_grandchild p1
+ Filter: ("left"(c, 3) ~ 'a1$'::text)
+ -> Seq Scan on permtest_grandchild p2
+(5 rows)
+
+reset session authorization;
+revoke all on permtest_parent from regress_no_child_access;
+drop role regress_no_child_access;
+drop table permtest_parent;
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 3af1bf3..ca21bbb 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -845,3 +845,27 @@ explain (costs off) select * from range_parted order by a,b,c;
explain (costs off) select * from range_parted order by a desc,b desc,c desc;
drop table range_parted;
+
+-- Test for checking that lack of access permissions for a child table and
+-- hence its statistics doesn't affect plan shapes when the query is on the
+-- parent table
+create table permtest_parent (a int, b text, c text) partition by list (a);
+create table permtest_child (b text, a int, c text) partition by list (b);
+create table permtest_grandchild (c text, b text, a int);
+alter table permtest_child attach partition permtest_grandchild for values in ('a');
+alter table permtest_parent attach partition permtest_child for values in (1);
+create index on permtest_parent (left(c, 3));
+insert into permtest_parent select 1, 'a', left(md5(i::text), 5) from generate_series(0, 100) i;
+analyze permtest_parent;
+explain (costs off) select * from permtest_parent p1 inner join permtest_parent p2 on p1.a = p2.a and p1.c ~ 'a1$';
+explain (costs off) select * from permtest_parent p1 inner join permtest_parent p2 on p1.a = p2.a and left(p1.c, 3) ~
'a1$';
+create role regress_no_child_access;
+revoke all on permtest_grandchild from regress_no_child_access;
+grant all on permtest_parent to regress_no_child_access;
+set session authorization regress_no_child_access;
+explain (costs off) select * from permtest_parent p1 inner join permtest_parent p2 on p1.a = p2.a and p1.c ~ 'a1$';
+explain (costs off) select * from permtest_parent p1 inner join permtest_parent p2 on p1.a = p2.a and left(p1.c, 3) ~
'a1$';
+reset session authorization;
+revoke all on permtest_parent from regress_no_child_access;
+drop role regress_no_child_access;
+drop table permtest_parent;
On Wed, Nov 27, 2019 at 3:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Amit Langote <amitlangote09@gmail.com> writes: > > If inh_root_relid meant that, it would no longer be useful to > > examine_variable. In examine_variable, we need to map a child table's > > relid to the relid of its root parent table. If the root parent > > itself is under a UNION ALL subquery parent, then inh_root_relid of > > all relations in that ancestry chain would point to the UNION ALL > > subquery parent, which is not what examine_variable would want to use, > > because it's really looking for the root "table". > > Hm, I see. Still, the definition seems quite ad-hoc and of uncertain > usefulness to any other use-case. Given that checking permissions for > access to an expression index's stats is a pretty uncommon thing to > be doing, I don't really want to let it drive the definition of a > new RelOptInfo field. > > The other reason that I'm on the warpath against this field is that > it makes the patch un-back-patchable, and I'd like to be able to fix > this problem in the back branches. Both arguments make sense. > Given the existence of the append_rel_array array, it's not really > difficult or expensive to use that to chain up to the root parent, > as in the attached simplified patch. We could only use this back > to v11 where append_rel_array was added, but that's still a lot > better than no back-patched fix at all. I agree. > I've not studied the test case too closely yet, other than to verify > that it does fail without the code fix :-). Other than that, though, > I think this patch is committable for v11 through HEAD. Thanks for committing. Regards, Amit