Re: ERROR: argument to pg_get_expr() must come from system catalogs

We had a report of the above error from a pgAdmin user testing
1.12.0b3 with PG 9.0b3. The (highly simplified) query below works fine
as a superuser:

SELECT pg_get_expr(proargdefaults, 'pg_catalog.pg_class'::regclass)
FROM pg_proc pr
LEFT OUTER JOIN pg_description des ON des.objoid=pr.oid

Run as a regular user though, we get the error. If I remove the join,
it works fine as the normal user. This is in a database owned by the
regular user.

Am I missing something obvious, or is there a bug here? pg_get_expr is
used pretty extensively in pgAdmin, so we're obviously keen to ensure
it works :-)

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

On 13 July 2010 16:31, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> We had a report of the above error from a pgAdmin user testing
> 1.12.0b3 with PG 9.0b3. The (highly simplified) query below works fine
> as a superuser:
>
> SELECT pg_get_expr(proargdefaults, 'pg_catalog.pg_class'::regclass)
>  FROM pg_proc pr
>  LEFT OUTER JOIN pg_description des ON des.objoid=pr.oid
>
> Run as a regular user though, we get the error. If I remove the join,
> it works fine as the normal user. This is in a database owned by the
> regular user.
>
> Am I missing something obvious, or is there a bug here? pg_get_expr is
> used pretty extensively in pgAdmin, so we're obviously keen to ensure
> it works :-)
>

I tested this on both beta2 and beta3 and can confirm that it works on
beta2 but produces the following error in beta3:

********** Error **********

ERROR: argument to pg_get_expr() must come from system catalogs
SQL state: 42501

Thom

On 13 July 2010 16:44, Thom Brown <thombrown(at)gmail(dot)com> wrote:
> On 13 July 2010 16:31, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> We had a report of the above error from a pgAdmin user testing
>> 1.12.0b3 with PG 9.0b3. The (highly simplified) query below works fine
>> as a superuser:
>>
>> SELECT pg_get_expr(proargdefaults, 'pg_catalog.pg_class'::regclass)
>>  FROM pg_proc pr
>>  LEFT OUTER JOIN pg_description des ON des.objoid=pr.oid
>>
>> Run as a regular user though, we get the error. If I remove the join,
>> it works fine as the normal user. This is in a database owned by the
>> regular user.
>>
>> Am I missing something obvious, or is there a bug here? pg_get_expr is
>> used pretty extensively in pgAdmin, so we're obviously keen to ensure
>> it works :-)
>>
>
> I tested this on both beta2 and beta3 and can confirm that it works on
> beta2 but produces the following error in beta3:
>
> ********** Error **********
>
> ERROR: argument to pg_get_expr() must come from system catalogs
> SQL state: 42501
>
> Thom
>

And here's why:
http://archives.postgresql.org/pgsql-committers/2010-06/msg00259.php

"stringToNode() and deparse_expression_pretty() crash on invalid input,
but we have nevertheless exposed them to users via pg_get_expr(). It would
be too much maintenance effort to rigorously check the input, so put a hack
in place instead to restrict pg_get_expr() so that the argument must come
from one of the system catalog columns known to contain valid expressions."

Thom

On Tue, Jul 13, 2010 at 4:48 PM, Thom Brown <thombrown(at)gmail(dot)com> wrote:
> On 13 July 2010 16:44, Thom Brown <thombrown(at)gmail(dot)com> wrote:
>> On 13 July 2010 16:31, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>> We had a report of the above error from a pgAdmin user testing
>>> 1.12.0b3 with PG 9.0b3. The (highly simplified) query below works fine
>>> as a superuser:
>>>
>>> SELECT pg_get_expr(proargdefaults, 'pg_catalog.pg_class'::regclass)
>>>  FROM pg_proc pr
>>>  LEFT OUTER JOIN pg_description des ON des.objoid=pr.oid
>>>
>>> Run as a regular user though, we get the error. If I remove the join,
>>> it works fine as the normal user. This is in a database owned by the
>>> regular user.
>>>
>>> Am I missing something obvious, or is there a bug here? pg_get_expr is
>>> used pretty extensively in pgAdmin, so we're obviously keen to ensure
>>> it works :-)
>>>
>>
>> I tested this on both beta2 and beta3 and can confirm that it works on
>> beta2 but produces the following error in beta3:
>>
>> ********** Error **********
>>
>> ERROR: argument to pg_get_expr() must come from system catalogs
>> SQL state: 42501
>>
>> Thom
>>
>
> And here's why:
> http://archives.postgresql.org/pgsql-committers/2010-06/msg00259.php
>
> "stringToNode() and deparse_expression_pretty() crash on invalid input,
> but we have nevertheless exposed them to users via pg_get_expr(). It would
> be too much maintenance effort to rigorously check the input, so put a hack
> in place instead to restrict pg_get_expr() so that the argument must come
> from one of the system catalog columns known to contain valid expressions."

Yeah, I recall that - but... the argument *is* coming from the system
catalogs, and why is the error only thrown for a non-superuser, when
the query includes the join?

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

On 13 July 2010 16:50, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> On Tue, Jul 13, 2010 at 4:48 PM, Thom Brown <thombrown(at)gmail(dot)com> wrote:
>> On 13 July 2010 16:44, Thom Brown <thombrown(at)gmail(dot)com> wrote:
>>> On 13 July 2010 16:31, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>> We had a report of the above error from a pgAdmin user testing
>>>> 1.12.0b3 with PG 9.0b3. The (highly simplified) query below works fine
>>>> as a superuser:
>>>>
>>>> SELECT pg_get_expr(proargdefaults, 'pg_catalog.pg_class'::regclass)
>>>>  FROM pg_proc pr
>>>>  LEFT OUTER JOIN pg_description des ON des.objoid=pr.oid
>>>>
>>>> Run as a regular user though, we get the error. If I remove the join,
>>>> it works fine as the normal user. This is in a database owned by the
>>>> regular user.
>>>>
>>>> Am I missing something obvious, or is there a bug here? pg_get_expr is
>>>> used pretty extensively in pgAdmin, so we're obviously keen to ensure
>>>> it works :-)
>>>>
>>>
>>> I tested this on both beta2 and beta3 and can confirm that it works on
>>> beta2 but produces the following error in beta3:
>>>
>>> ********** Error **********
>>>
>>> ERROR: argument to pg_get_expr() must come from system catalogs
>>> SQL state: 42501
>>>
>>> Thom
>>>
>>
>> And here's why:
>> http://archives.postgresql.org/pgsql-committers/2010-06/msg00259.php
>>
>> "stringToNode() and deparse_expression_pretty() crash on invalid input,
>> but we have nevertheless exposed them to users via pg_get_expr(). It would
>> be too much maintenance effort to rigorously check the input, so put a hack
>> in place instead to restrict pg_get_expr() so that the argument must come
>> from one of the system catalog columns known to contain valid expressions."
>
> Yeah, I recall that - but... the argument *is* coming from the system
> catalogs, and why is the error only thrown for a non-superuser, when
> the query includes the join?
>

I works if you use pr.proargdefaults so not unresolvable. Maybe it's
because it can't tell where the column's coming from at that point?

Thom

On Tue, Jul 13, 2010 at 4:56 PM, Thom Brown <thombrown(at)gmail(dot)com> wrote:

> I works if you use pr.proargdefaults so not unresolvable.  Maybe it's
> because it can't tell where the column's coming from at that point?

Hmm, so it does. It still seems like a bug though - why should it be
able to resolve the column based on whether you're a superuser or not,
just because it's not qualified?

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

On 13 July 2010 17:00, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> On Tue, Jul 13, 2010 at 4:56 PM, Thom Brown <thombrown(at)gmail(dot)com> wrote:
>
>> I works if you use pr.proargdefaults so not unresolvable.  Maybe it's
>> because it can't tell where the column's coming from at that point?
>
> Hmm, so it does. It still seems like a bug though - why should it be
> able to resolve the column based on whether you're a superuser or not,
> just because it's not qualified?
>

Yeah see what you mean. Seems like a bug.

Thom

On 13 July 2010 17:01, Thom Brown <thombrown(at)gmail(dot)com> wrote:
> On 13 July 2010 17:00, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> On Tue, Jul 13, 2010 at 4:56 PM, Thom Brown <thombrown(at)gmail(dot)com> wrote:
>>
>>> I works if you use pr.proargdefaults so not unresolvable.  Maybe it's
>>> because it can't tell where the column's coming from at that point?
>>
>> Hmm, so it does. It still seems like a bug though - why should it be
>> able to resolve the column based on whether you're a superuser or not,
>> just because it's not qualified?
>>
>
> Yeah see what you mean.  Seems like a bug.
>
> Thom
>

And yes, as a result PgAdmin becomes very upset when not a superuser.
:( Kind of a show-stopper.

Thom

Dave Page <dpage(at)pgadmin(dot)org> writes:
> We had a report of the above error from a pgAdmin user testing
> 1.12.0b3 with PG 9.0b3. The (highly simplified) query below works fine
> as a superuser:

> SELECT pg_get_expr(proargdefaults, 'pg_catalog.pg_class'::regclass)
> FROM pg_proc pr
> LEFT OUTER JOIN pg_description des ON des.objoid=pr.oid

> Run as a regular user though, we get the error. If I remove the join,
> it works fine as the normal user. This is in a database owned by the
> regular user.

> Am I missing something obvious, or is there a bug here?

Yeah, it's a bug. The code Heikki added in parse_expr.c isn't allowing
for the possibility of join alias Vars. (There's another problem with
it too, which is you can trivially circumvent the check, just by
creating an operator based on pg_get_expr ...)

I wasn't terribly happy with that approach to begin with. I think we
need to rethink.

regards, tom lane

On 13/07/10 21:36, Tom Lane wrote:
> Dave Page<dpage(at)pgadmin(dot)org> writes:
>> We had a report of the above error from a pgAdmin user testing
>> 1.12.0b3 with PG 9.0b3. The (highly simplified) query below works fine
>> as a superuser:
>
>> SELECT pg_get_expr(proargdefaults, 'pg_catalog.pg_class'::regclass)
>> FROM pg_proc pr
>> LEFT OUTER JOIN pg_description des ON des.objoid=pr.oid
>
>> Run as a regular user though, we get the error. If I remove the join,
>> it works fine as the normal user. This is in a database owned by the
>> regular user.
>
>> Am I missing something obvious, or is there a bug here?
>
> Yeah, it's a bug. The code Heikki added in parse_expr.c isn't allowing
> for the possibility of join alias Vars.

Hmm, shouldn't be too hard to fix..

> (There's another problem with
> it too, which is you can trivially circumvent the check, just by
> creating an operator based on pg_get_expr ...)

Yeah, good catch. That should be fixable too..

> I wasn't terribly happy with that approach to begin with. I think we
> need to rethink.

Do you want to go ahead with your plan of changing what's passed in
FuncInfo? I won't object if you want to do it, but I wouldn't feel
comfortable with backporting such big changes myself.

If we continue with the approach I took, we should implement the
suggestion to create a new data type for this in 9.1. That would be more
waterproof than the changes I made, if we introduce new ways to call
functions in the future.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Jul 16, 2010, at 2:27 AM, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> If we continue with the approach I took, we should implement the suggestion to create a new data type for this in 9.1. That would be more waterproof than the changes I made, if we introduce new ways to call functions in the future.

The downside is that it might cause the approach used in the older releases to get less testing.

...Robert

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> On 13/07/10 21:36, Tom Lane wrote:
>> I wasn't terribly happy with that approach to begin with. I think we
>> need to rethink.

> Do you want to go ahead with your plan of changing what's passed in
> FuncInfo? I won't object if you want to do it, but I wouldn't feel
> comfortable with backporting such big changes myself.

I will take a look at it, but not right away. Since we have no
near-term plans for new minor releases, I don't think it's urgent.

> If we continue with the approach I took, we should implement the
> suggestion to create a new data type for this in 9.1. That would be more
> waterproof than the changes I made, if we introduce new ways to call
> functions in the future.

Agreed, that seems like a better solution going forward than either of
the others.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Jul 16, 2010, at 2:27 AM, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> If we continue with the approach I took, we should implement the suggestion to create a new data type for this in 9.1. That would be more waterproof than the changes I made, if we introduce new ways to call functions in the future.

> The downside is that it might cause the approach used in the older releases to get less testing.

I hope we can get a better fix into the next 9.0 beta, so it will get
some field testing before any back-branch minor releases happen.

regards, tom lane

I wrote:
> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
>> Do you want to go ahead with your plan of changing what's passed in
>> FuncInfo? I won't object if you want to do it, but I wouldn't feel
>> comfortable with backporting such big changes myself.

> I will take a look at it, but not right away.

I spent some more time looking at this, and decided that it's probably
not really a good plan after all. It would be a significant amount of
work for what's basically a dead-end back-branch-only patch, given that
we intend to go with the special-datatype solution for 9.1 and up.
The work aspect is bad enough, but I also share Robert's nervousness
about whether it would get enough testing before going out in releases.
The code that would be getting changed has mutated quite a lot over the
years, to the point where we'd almost have to test each branch
separately to have much confidence in the fixes.

Please see what you can do with plugging the holes in the existing
patch, instead.

regards, tom lane

Tom Lane wrote:
> I wrote:
> > Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> >> Do you want to go ahead with your plan of changing what's passed in
> >> FuncInfo? I won't object if you want to do it, but I wouldn't feel
> >> comfortable with backporting such big changes myself.
>
> > I will take a look at it, but not right away.
>
> I spent some more time looking at this, and decided that it's probably
> not really a good plan after all. It would be a significant amount of
> work for what's basically a dead-end back-branch-only patch, given that
> we intend to go with the special-datatype solution for 9.1 and up.
> The work aspect is bad enough, but I also share Robert's nervousness
> about whether it would get enough testing before going out in releases.
> The code that would be getting changed has mutated quite a lot over the
> years, to the point where we'd almost have to test each branch
> separately to have much confidence in the fixes.
>
> Please see what you can do with plugging the holes in the existing
> patch, instead.

I can confirm that this bug, introduced in a minor release, is making
pgAdmin look bad, with repeated popup error boxes while viewing user
objects. Grewsome screenshot attached. ;-)

Are we basically leaving pgAdmin in this state until we come up with a
fix and need a new minor release? We pride ourselves in not introducing
breakage in minor releases, but it has certainly happened in this case,
and it is making pgAdmin look bad. Dave, is there a hack you can add to
pgAdmin to work around the join issue until we can fix the backend?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Wed, Jul 28, 2010 at 4:54 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Are we basically leaving pgAdmin in this state until we come up with a
> fix and need a new minor release?  We pride ourselves in not introducing
> breakage in minor releases, but it has certainly happened in this case,
> and it is making pgAdmin look bad.  Dave, is there a hack you can add to
> pgAdmin to work around the join issue until we can fix the backend?

It wouldn't make much difference if there was - the majority of people
won't get it until they upgrade their server anyway.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

Dave Page <dpage(at)pgadmin(dot)org> writes:
> We had a report of the above error from a pgAdmin user testing
> 1.12.0b3 with PG 9.0b3. The (highly simplified) query below works fine
> as a superuser:

> SELECT pg_get_expr(proargdefaults, 'pg_catalog.pg_class'::regclass)
> FROM pg_proc pr
> LEFT OUTER JOIN pg_description des ON des.objoid=pr.oid

> Run as a regular user though, we get the error.

I've applied a (rather hurried) patch for this for 9.0beta4.

regards, tom lane

On Fri, Jul 30, 2010 at 12:17 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dave Page <dpage(at)pgadmin(dot)org> writes:
>> We had a report of the above error from a pgAdmin user testing
>> 1.12.0b3 with PG 9.0b3. The (highly simplified) query below works fine
>> as a superuser:
>
>> SELECT pg_get_expr(proargdefaults, 'pg_catalog.pg_class'::regclass)
>>   FROM pg_proc pr
>>   LEFT OUTER JOIN pg_description des ON des.objoid=pr.oid
>
>> Run as a regular user though, we get the error.
>
> I've applied a (rather hurried) patch for this for 9.0beta4.

Thanks. Bruce seemed to think it affected 8.4.4 as well - would that
be the case, or is it something else?

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

Dave Page <dpage(at)pgadmin(dot)org> writes:
> On Fri, Jul 30, 2010 at 12:17 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I've applied a (rather hurried) patch for this for 9.0beta4.

> Thanks. Bruce seemed to think it affected 8.4.4 as well - would that
> be the case, or is it something else?

He's mistaken. The bug is in all the branches, but there have been no
releases with it except 9.0beta3. I will work on back-patching the
older branches this morning.

regards, tom lane

Tom Lane wrote:
> Dave Page <dpage(at)pgadmin(dot)org> writes:
> > On Fri, Jul 30, 2010 at 12:17 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> I've applied a (rather hurried) patch for this for 9.0beta4.
>
> > Thanks. Bruce seemed to think it affected 8.4.4 as well - would that
> > be the case, or is it something else?
>
> He's mistaken. The bug is in all the branches, but there have been no
> releases with it except 9.0beta3. I will work on back-patching the
> older branches this morning.

I was using 8.4.X CVS with pgAdmin 8.4 and was seeing the error. Is the
error in 8.4.X CVS?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> Dave Page <dpage(at)pgadmin(dot)org> writes:
>>> Thanks. Bruce seemed to think it affected 8.4.4 as well - would that
>>> be the case, or is it something else?
>>
>> He's mistaken. The bug is in all the branches, but there have been no
>> releases with it except 9.0beta3. I will work on back-patching the
>> older branches this morning.

> I was using 8.4.X CVS with pgAdmin 8.4 and was seeing the error. Is the
> error in 8.4.X CVS?

It was in CVS, but it never got into a release.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> Dave Page <dpage(at)pgadmin(dot)org> writes:
> >>> Thanks. Bruce seemed to think it affected 8.4.4 as well - would that
> >>> be the case, or is it something else?
> >>
> >> He's mistaken. The bug is in all the branches, but there have been no
> >> releases with it except 9.0beta3. I will work on back-patching the
> >> older branches this morning.
>
> > I was using 8.4.X CVS with pgAdmin 8.4 and was seeing the error. Is the
> > error in 8.4.X CVS?
>
> It was in CVS, but it never got into a release.

OK, that explains why I see it and no one else does --- never mind. ;-)

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +