Thread: Bug with view definitions?

Bug with view definitions?

From
Justin Clift
Date:
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



Re: Bug with view definitions?

From
Dennis Bjorklund
Date:
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



Re: Bug with view definitions?

From
Dennis Bjorklund
Date:
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



Re: Bug with view definitions?

From
Justin Clift
Date:
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




Re: Bug with view definitions?

From
Dennis Bjorklund
Date:
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



Re: Bug with view definitions?

From
Bruno Wolff III
Date:
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.


Re: Bug with view definitions?

From
Tom Lane
Date:
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


Re: Bug with view definitions?

From
Andreas Pflug
Date:
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




Re: Bug with view definitions?

From
Tom Lane
Date:
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


Re: Bug with view definitions?

From
Justin Clift
Date:
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



Re: Bug with view definitions?

From
Christopher Kings-Lynne
Date:
> 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



Re: Bug with view definitions?

From
Tom Lane
Date:
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


Re: Bug with view definitions?

From
Tom Lane
Date:
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


Re: Bug with view definitions?

From
Christopher Kings-Lynne
Date:
> 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...



Re: Bug with view definitions?

From
Christopher Kings-Lynne
Date:
>>  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



Re: Bug with view definitions?

From
Tom Lane
Date:
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


Re: Bug with view definitions?

From
Christopher Kings-Lynne
Date:
>>So what are we going to do about it?
> 
> 
> Fix it, of course.

I just wanted to make sure it happened :)


Re: Bug with view definitions?

From
Andreas Pflug
Date:
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,



Re: Bug with view definitions?

From
Andreas Pflug
Date:
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