Thread: Bug with view definitions?
Hi guys, Not sure if this is a known issue or not, but I think I may have found a bug with the way view definitions are shown... at least in psql. Using 7.5 development CVS (as of a few hours ago) or even 7.4.3, if I connect using it's version of psql to a database (of the same version), then use psql to view the information_schema.constraint_columns_usage view, it gives me this definition: *********** mydb=# \d information_schema.constraint_column_usage View "information_schema.constraint_column_usage" Column | Type | Modifiers --------------------+-----------------------------------+----------- table_catalog | information_schema.sql_identifier| table_schema | information_schema.sql_identifier | table_name | information_schema.sql_identifier| column_name | information_schema.sql_identifier | constraint_catalog | information_schema.sql_identifier| constraint_schema | information_schema.sql_identifier | constraint_name | information_schema.sql_identifier| View definition: SELECT current_database()::information_schema.sql_identifier AS table_catalog, x.tblschema::information_schema.sql_identifier AS table_schema, x.tblname::information_schema.sql_identifier AS table_name, x.colname::information_schema.sql_identifier AS column_name, current_database()::information_schema.sql_identifier AS constraint_catalog, x.cstrschema::information_schema.sql_identifier AS constraint_schema, x.cstrname::information_schema.sql_identifier AS constraint_name FROM ( SELECT DISTINCT nr.nspname, r.relname, r.relowner, a.attname, nc.nspname, c.conname FROM pg_namespace nr, pg_class r, pg_attribute a, pg_depend d, pg_namespace nc, pg_constraint c WHERE nr.oid = r.relnamespace AND r.oid = a.attrelid AND d.refclassid = 'pg_class'::regclass::oid AND d.refobjid = r.oid AND d.refobjsubid = a.attnum AND d.classid = 'pg_constraint'::regclass::oid AND d.objid = c.oid AND c.connamespace = nc.oid AND c.contype = 'c'::"char" AND r.relkind = 'r'::"char" AND NOT a.attisdropped ORDER BY nr.nspname, r.relname, r.relowner, a.attname, nc.nspname, c.conname UNION ALL SELECT nr.nspname, r.relname, r.relowner, a.attname, nc.nspname, c.conname FROM pg_namespace nr, pg_class r,pg_attribute a, pg_namespace nc, pg_constraint c, information_schema._pg_keypositions() pos(n) WHERE nr.oid = r.relnamespace AND r.oid = a.attrelid AND nc.oid= c.connamespace AND CASE WHEN c.contype = 'f'::"char" THEN r.oid = c.confrelid AND c.confkey[pos.n] = a.attnum ELSE r.oid = c.conrelid AND c.conkey[pos.n] = a.attnum END AND NOT a.attisdroppedAND (c.contype = 'p'::"char" OR c.contype = 'u'::"char" OR c.contype = 'f'::"char") AND r.relkind = 'r'::"char") x(tblschema, tblname, tblowner, colname, cstrschema, cstrname), pg_user u WHERE x.tblowner = u.usesysid AND u.usename = "current_user"(); mydb=# *********** However, when I use this definition (cut-n-paste style to avoid mistakes) to create the view anew (even with a different name, etc), then it gives me an error: *********** mydb=# \e ERROR: parse error at or near "ALL" at character 1105 ERROR: parse error at or near "ALL" at character 1105 LINE 6: UNION ALL ^ mydb=# *********** I haven't come across this before, and am having the same problem with pgAdmin3 as well, as it supplies the exact same definition of the view. I think I'm doing everything right here, could this be a bug with PG? Regards and best wishes, Justin Clift
On Thu, 1 Jul 2004, Justin Clift wrote: > Not sure if this is a known issue or not, but I think I may have found a > bug with the way view definitions are shown... at least in psql. > > Using 7.5 development CVS (as of a few hours ago) or even 7.4.3, if I > connect using it's version of psql to a database (of the same version), > then use psql to view the information_schema.constraint_columns_usage > view, it gives me this definition: [long view defintion from \d information_schema.constraint_column_usage] The thing that does not work is that the SELECT to the left of the UNION ALL needs to be put inside (), then it works and the parser can parse it. Looking at the doc page it looks like the () should not be needed but one need to check real grammar (specification) to really know how it should be parsed. -- /Dennis Björklund
On Thu, 1 Jul 2004, Dennis Bjorklund wrote: >> \d information_schema.constraint_column_usage > The thing that does not work is that the SELECT to the left of the UNION > ALL needs to be put inside (), then it works and the parser can parse it. > > Looking at the doc page it looks like the () should not be needed but one > need to check real grammar (specification) to really know how it should be > parsed. Looking again at the doc and the example I now know why it can't parse it. The example when simplified is: SELECT * FROM (select 1 ORDER BY 1 UNION ALL select 2) AS x; and it does not parse since the there is an ORDER BY in the first query. If we look at the doc page then the UNION comes before the ORDER BY, so it is in fact an invalid query (I've not checke the standard, just the select doc page). If you put a () around the first (inner) select it all works. But why is the order by there at all? The order of the rows from the UNION ALL can (in theory) be random anyway, right? I've still not checked any code. I don't even know what part of pg it is that produce that bad SQL. The view itself works, so it must be the pretty printer that is broken (where ever that is hidden away in the code). -- /Dennis Björklund
Dennis Bjorklund wrote: <snip> > I've still not checked any code. I don't even know what part of pg it is > that produce that bad SQL. The view itself works, so it must be the pretty > printer that is broken (where ever that is hidden away in the code). Thanks Dennis. So, it's definitely a bug then. I wasn't sure if it was PG or me. :) Um, as I'm not up to the task of fixing it, is this something you or someone else would be interested in? Regards and best wishes, Justin Clift
On Thu, 1 Jul 2004, Bruno Wolff III wrote: > If DISTINCT ON or LIMIT was used in inner select, then the ORDER BY would > be relevant; so you can't just blindly remove ORDER BY when it is part of > a union. Of course, but in this case with this view there wasn't any such. It can still be usable since we know how pg sorts and this is an internal query. The real bug is in the pretty printer anyway. I was just surprised to see the order by inside the union and not on the outside where it belongs (and just moving it out in this case wont produce exactly the same result). -- /Dennis Björklund
On Thu, Jul 01, 2004 at 15:19:32 +0200, Dennis Bjorklund <db@zigo.dhs.org> wrote: > > Looking again at the doc and the example I now know why it can't parse > it. The example when simplified is: > > SELECT * > FROM (select 1 ORDER BY 1 > UNION ALL > select 2) AS x; > > and it does not parse since the there is an ORDER BY in the first query. > If we look at the doc page then the UNION comes before the ORDER BY, so it > is in fact an invalid query (I've not checke the standard, just the select > doc page). > > If you put a () around the first (inner) select it all works. But why > is the order by there at all? The order of the rows from the UNION ALL can > (in theory) be random anyway, right? If DISTINCT ON or LIMIT was used in inner select, then the ORDER BY would be relevant; so you can't just blindly remove ORDER BY when it is part of a union.
Dennis Bjorklund <db@zigo.dhs.org> writes:
> The view itself works, so it must be the pretty
> printer that is broken (where ever that is hidden away in the code).
Yeah, I think this is due to overenthusiastic removal of parentheses.
I believe if you pg_dump the view you will get a correctly parenthesized
version, because pg_dump does not trust the pretty printer (for reasons
that should now be obvious...)
regards, tom lane
Justin Clift wrote: > Dennis Bjorklund wrote: > <snip> > >> I've still not checked any code. I don't even know what part of pg it >> is that produce that bad SQL. The view itself works, so it must be >> the pretty printer that is broken (where ever that is hidden away in >> the code). > > > Thanks Dennis. > > So, it's definitely a bug then. I wasn't sure if it was PG or me. :) The source of information_schema.constraint_column_usage in backend/catalog/information_schema.sql doesn't have the ORDER BY clause, but pg_get_viewdef finds one. A quick glance at adt/ruleutils.c doesn't show an obvious problem, so the inner query somehow acquired a sortClause. Regards, Andreas
Dennis Bjorklund <db@zigo.dhs.org> writes:
> On Thu, 1 Jul 2004, Bruno Wolff III wrote:
>> If DISTINCT ON or LIMIT was used in inner select, then the ORDER BY would
>> be relevant; so you can't just blindly remove ORDER BY when it is part of
>> a union.
> Of course, but in this case with this view there wasn't any such. It can
> still be usable since we know how pg sorts and this is an internal query.
> The real bug is in the pretty printer anyway. I was just surprised to see
> the order by inside the union and not on the outside where it belongs (and
> just moving it out in this case wont produce exactly the same result).
Actually, if you look at the source code (information_schema.sql) there
is no ORDER BY in it, only a DISTINCT. The ORDER BY gets added by the
parser to help implement the DISTINCT. Sooner or later we should look
at suppressing the added ORDER BY when displaying the view.
regards, tom lane
Tom Lane wrote: <snip> > Actually, if you look at the source code (information_schema.sql) there > is no ORDER BY in it, only a DISTINCT. The ORDER BY gets added by the > parser to help implement the DISTINCT. Sooner or later we should look > at suppressing the added ORDER BY when displaying the view. If someone fixes this can we make sure it goes into 7.4.4 as well (if it's not a drastic code change)? It's not a data corrupting bug but it's stopping view definitions from "working as advertised" which is bad if you're used to being able to rely on them. :-/ For now, I'll personally use the pg_dump version of the query, or maybe see if the one in backend/catalog/information_schema.sql can be run directly. :) Regards and best wishes, Justin Clift
> It's not a data corrupting bug but it's stopping view definitions from > "working as advertised" which is bad if you're used to being able to > rely on them. :-/ Hmm, is this wrong on line 2085 of src/backend/adt/utils/ruleutils.c: need_paren = (PRETTY_PAREN(context) ? !IsA(op->rarg, RangeTblRef) : true); The variable need_paren needs to be true in the case of the information_schema query, but I'm not sure what to add as the additional clause? Chris
Justin Clift <jc@telstra.net> writes:
> Tom Lane wrote:
>> Actually, if you look at the source code (information_schema.sql) there
>> is no ORDER BY in it, only a DISTINCT. The ORDER BY gets added by the
>> parser to help implement the DISTINCT. Sooner or later we should look
>> at suppressing the added ORDER BY when displaying the view.
> If someone fixes this can we make sure it goes into 7.4.4 as well (if
> it's not a drastic code change)?
The thoughts I had for fixing it involved adding a field to SortClause
nodes to show whether they came from an actual user clause or were added
by the parser. This would be an initdb-forcing change and thus
unsuitable for a backpatch to 7.4 ...
> It's not a data corrupting bug but it's stopping view definitions from
> "working as advertised" which is bad if you're used to being able to
> rely on them. :-/
No, the pretty-printer's failure to add parens here is a different bug.
That we could fix without a data structure change. It's just a matter
of figuring out exactly where it's being too permissive about dropping
parens.
regards, tom lane
Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
> Hmm, is this wrong on line 2085 of src/backend/adt/utils/ruleutils.c:
> need_paren = (PRETTY_PAREN(context) ?
> !IsA(op->rarg, RangeTblRef) : true);
In a quick glance this code seems close to completely brain dead :-(
For one thing, why isn't it making separate determinations about whether
the left and right inputs of the UNION (resp INTERSECT or EXCEPT)
operator need to be parenthesized? After that maybe we could figure out
what the individual decisions need to be.
regards, tom lane
> In a quick glance this code seems close to completely brain dead :-( > For one thing, why isn't it making separate determinations about whether > the left and right inputs of the UNION (resp INTERSECT or EXCEPT) > operator need to be parenthesized? After that maybe we could figure out > what the individual decisions need to be. Well it's the work of 5 seconds to have two separate tests. What I don't feel confident in doing is getting the tests themselves right. That's why I can't really do it...
>> need_paren = (PRETTY_PAREN(context) ? >> !IsA(op->rarg, RangeTblRef) : true); > > > In a quick glance this code seems close to completely brain dead :-( > For one thing, why isn't it making separate determinations about whether > the left and right inputs of the UNION (resp INTERSECT or EXCEPT) > operator need to be parenthesized? After that maybe we could figure out > what the individual decisions need to be. So what are we going to do about it? Was it one of the pgAdmin guys who wrote it in the first place? Chris
Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
>> In a quick glance this code seems close to completely brain dead :-(
> So what are we going to do about it?
Fix it, of course.
(Note I only fixed the parenthesization, I take no responsibility for
the still-pretty-brain-dead layout.)
regards, tom lane
>>So what are we going to do about it? > > > Fix it, of course. I just wanted to make sure it happened :)
Christopher Kings-Lynne wrote: >>> need_paren = (PRETTY_PAREN(context) ? >>> !IsA(op->rarg, RangeTblRef) : true); >> >> >> >> In a quick glance this code seems close to completely brain dead :-( >> For one thing, why isn't it making separate determinations about whether >> the left and right inputs of the UNION (resp INTERSECT or EXCEPT) >> operator need to be parenthesized? After that maybe we could figure out >> what the individual decisions need to be. > > > So what are we going to do about it? > > Was it one of the pgAdmin guys who wrote it in the first place? Yep, me. It was still on my radar to fix; not surprising, Tom was faster. I'll have a look at the "braindead" issue. Regards,
Andreas Pflug wrote: > Christopher Kings-Lynne wrote: > >>>> need_paren = (PRETTY_PAREN(context) ? >>>> !IsA(op->rarg, RangeTblRef) : true); >>> >>> >>> >>> >>> In a quick glance this code seems close to completely brain dead :-( >> This probably was about catching expr_A UNION (expr_B INTERSECT expr_C) cases, falsely assuming left-to-right won't ever need parentheses. Apparently the current version already fixes this completely, suppressing parentheses also with non-pretty. Regards, Andreas