Thread: Re: [BUGS] BUG #3829: Wrong index reporting from pgAdmin III (v1.8.0 rev 6766-6767)

Hello

it's pgAdmin bug. please report it there

Regards
Pavel Stehule

On 19/12/2007, Boonchai <boonchai@xsidekick.com> wrote:
>
> The following bug has been logged online:
>
> Bug reference:      3829
> Logged by:          Boonchai
> Email address:      boonchai@xsidekick.com
> PostgreSQL version: 8.3 beta 4
> Operating system:   windows XP
> Description:        Wrong index reporting from pgAdmin III (v1.8.0 rev
> 6766-6767)
> Details:
>
> I created a table by
>
> CREATE TABLE test_desc
> (
>   id integer,
>   f1 character(10),
>   f2 character(20)
> )
>
> case 1 :
> Created index by
>
> CREATE INDEX test_desc_idx1
>   ON test_desc
>   USING btree
>   (f1, f2 desc)
>
> but pgAdmin reported like this
>
> CREATE INDEX test_desc_idx
>   ON test_desc
>   USING btree
>   (f1 DESC, f2 DESC)
>
>
> case 2:
> Created another index by
>
> CREATE INDEX test_desc_idx2
>   ON test_desc
>   USING btree
>   (f1 desc, f2);
>
> but pgAdmin reported
>
> CREATE INDEX test_desc_idx2
>   ON test_desc
>   USING btree
>   (f1 DESC,  DESCf2);
>
> Anyway, these 2 indexes are corrected, seen from pg_indexes.
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend
>

Pavel Stehule wrote:
> Hello
> 
> it's pgAdmin bug. please report it there

Actually it doesn't appear to be...

Using the OPs example code on 8.3 beta 4:

>> CREATE INDEX test_desc_idx1
>>   ON test_desc
>>   USING btree
>>   (f1, f2 desc)
>>
>> CREATE INDEX test_desc_idx2
>>   ON test_desc
>>   USING btree
>>   (f1 desc, f2);

postgres=# select pg_get_indexdef('test_desc_idx1'::regclass, 0, true);                         pg_get_indexdef
--------------------------------------------------------------------CREATE INDEX test_desc_idx1 ON test_desc USING
btree(f1, f2 DESC)
 
(1 row)

postgres=# select pg_get_indexdef('test_desc_idx1'::regclass, 1, true);pg_get_indexdef
-----------------f1 DESC
(1 row)

postgres=# select pg_get_indexdef('test_desc_idx1'::regclass, 2, true);pg_get_indexdef
-----------------f2 DESC
(1 row)

postgres=# select pg_get_indexdef('test_desc_idx2'::regclass, 0, true);                         pg_get_indexdef
--------------------------------------------------------------------CREATE INDEX test_desc_idx2 ON test_desc USING
btree(f1 DESC, f2)
 
(1 row)

postgres=# select pg_get_indexdef('test_desc_idx2'::regclass, 1, true);pg_get_indexdef
-----------------f1 DESC
(1 row)

postgres=# select pg_get_indexdef('test_desc_idx2'::regclass, 2, true);pg_get_indexdef
----------------- DESCf2
(1 row)

Looks like pg_get_indexdef is unwell :-(

/D

NikhilS <nikkhils@gmail.com> writes:
>> Looks like pg_get_indexdef is unwell :-(

> yes, it was unwell in the area where the amcanorder was being processed. The
> attached patch should fix this.

Hm, there is a definitional issue here.  Should pg_get_indexdef print
this stuff at all when colno is nonzero?  The header comment says that
it is to return the column's variable or expression only.  The existing
code suppresses the opclass in this case, which to me suggests that it
should suppress DESC/ASC as well.  Which is not what Nikhil's patch
does.

Dave, I think we put in this variant of the function for pgAdmin ---
what does pgAdmin need?
        regards, tom lane

Tom Lane wrote:
> NikhilS <nikkhils@gmail.com> writes:
>>> Looks like pg_get_indexdef is unwell :-(
> 
>> yes, it was unwell in the area where the amcanorder was being processed. The
>> attached patch should fix this.
> 
> Hm, there is a definitional issue here.  Should pg_get_indexdef print
> this stuff at all when colno is nonzero?  The header comment says that
> it is to return the column's variable or expression only.  The existing
> code suppresses the opclass in this case, which to me suggests that it
> should suppress DESC/ASC as well.  Which is not what Nikhil's patch
> does.
> 
> Dave, I think we put in this variant of the function for pgAdmin ---
> what does pgAdmin need?

More is better for us - it saves an ugly query that will get uglier if
we need to figure out ASC/DESC here too :-)

I agree that we should have all or nothing though, so I'd like to see
ASC/DESC and opclass please.

/D

Dave Page <dpage@postgresql.org> writes:
> Tom Lane wrote:
>> Hm, there is a definitional issue here.  Should pg_get_indexdef print
>> this stuff at all when colno is nonzero?
>> ...
>> Dave, I think we put in this variant of the function for pgAdmin ---
>> what does pgAdmin need?

> More is better for us - it saves an ugly query that will get uglier if
> we need to figure out ASC/DESC here too :-)
> I agree that we should have all or nothing though, so I'd like to see
> ASC/DESC and opclass please.

I dug through the archives and found that we've had this discussion
before ;-).  The basic argument for having the per-column form of
pg_get_indexdef do what it does was that it's unreasonable for
client-side code to try to disassemble an expression tree string,
whereas extracting opclass info is a relatively straightforward
exercise in joining.  There are several past threads about this:

http://archives.postgresql.org/pgsql-hackers/2003-07/msg00083.php
http://archives.postgresql.org/pgsql-general/2005-11/msg01106.php
http://archives.postgresql.org/pgsql-hackers/2006-06/msg00576.php

As of 8.3 that expands into also having to know the meaning of the
bits in indoption[], which is kind of annoying, but not even close
to being in the same league as reverse-compiling expressions.

I think the current API expectation for pg_get_indexdef is that
it produces only the index column/expression, and that we are
very likely to break client-side code if we change that.

I don't have any objection to providing an additional new API that
includes the opclass and ASC/DESC decoration in the output ... other
than that I think it's a bit too late for 8.3; adding a function
would mean forcing initdb, and I don't see any reasonable way to
shoehorn two behaviors into the existing function signature.

Just out of curiosity, why is pgAdmin doing it this way at all?
Seems it would be a lot easier to use the all-columns form of
pg_get_indexdef than to cons up the display from fetches of each
column individually.
        regards, tom lane

Tom Lane wrote:
> I dug through the archives and found that we've had this discussion
> before ;-).  The basic argument for having the per-column form of
> pg_get_indexdef do what it does was that it's unreasonable for
> client-side code to try to disassemble an expression tree string,
> whereas extracting opclass info is a relatively straightforward
> exercise in joining.  There are several past threads about this:
> 
> http://archives.postgresql.org/pgsql-hackers/2003-07/msg00083.php
> http://archives.postgresql.org/pgsql-general/2005-11/msg01106.php
> http://archives.postgresql.org/pgsql-hackers/2006-06/msg00576.php

Ah, that takes me back :-)

> As of 8.3 that expands into also having to know the meaning of the
> bits in indoption[], which is kind of annoying, but not even close
> to being in the same league as reverse-compiling expressions.

Agreed.

> I think the current API expectation for pg_get_indexdef is that
> it produces only the index column/expression, and that we are
> very likely to break client-side code if we change that.

I'm sure there are far more catalog/API changes that'll affect any app
likely to be using this form of pg_get_indexdef, but I can live without
adding another.

> I don't have any objection to providing an additional new API that
> includes the opclass and ASC/DESC decoration in the output ... other
> than that I think it's a bit too late for 8.3; adding a function
> would mean forcing initdb, and I don't see any reasonable way to
> shoehorn two behaviors into the existing function signature.

Agreed - now is not the time to be adding new functions.

> Just out of curiosity, why is pgAdmin doing it this way at all?
> Seems it would be a lot easier to use the all-columns form of
> pg_get_indexdef than to cons up the display from fetches of each
> column individually.

We use the data in various UI elements as well as for reverse
engineering the SQL - it's easier to get it broken down than to parse it
back out of the complete definition.

/D

Dave Page <dpage@postgresql.org> writes:
> Tom Lane wrote:
>> Just out of curiosity, why is pgAdmin doing it this way at all?
>> Seems it would be a lot easier to use the all-columns form of
>> pg_get_indexdef than to cons up the display from fetches of each
>> column individually.

> We use the data in various UI elements as well as for reverse
> engineering the SQL - it's easier to get it broken down than to parse it
> back out of the complete definition.

Seems like all the more argument for having functions that extract
single pieces of information, rather than several pieces (especially
if some pieces get left off when default).

For the moment I've reverted pg_get_indexdef() to its prior behavior
of printing only the index column key or expression when colno!=0.
We can look at having another function to do the other thing in 8.4.
        regards, tom lane

Tom Lane wrote:
>> We use the data in various UI elements as well as for reverse
>> engineering the SQL - it's easier to get it broken down than to parse it
>> back out of the complete definition.
> 
> Seems like all the more argument for having functions that extract
> single pieces of information, rather than several pieces (especially
> if some pieces get left off when default).

Well it might well have been if the pgAdmin code didn't already work and 
it wasn't so close to release :-)

> For the moment I've reverted pg_get_indexdef() to its prior behavior
> of printing only the index column key or expression when colno!=0.
> We can look at having another function to do the other thing in 8.4.

Thanks.

/D

Added to TODO:

* Add a function like pg_get_indexdef() that report more detailed index information
 http://archives.postgresql.org/pgsql-bugs/2007-12/msg00166.php


---------------------------------------------------------------------------

Dave Page wrote:
> Tom Lane wrote:
> >> We use the data in various UI elements as well as for reverse
> >> engineering the SQL - it's easier to get it broken down than to parse it
> >> back out of the complete definition.
> > 
> > Seems like all the more argument for having functions that extract
> > single pieces of information, rather than several pieces (especially
> > if some pieces get left off when default).
> 
> Well it might well have been if the pgAdmin code didn't already work and 
> it wasn't so close to release :-)
> 
> > For the moment I've reverted pg_get_indexdef() to its prior behavior
> > of printing only the index column key or expression when colno!=0.
> > We can look at having another function to do the other thing in 8.4.
> 
> Thanks.
> 
> /D
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +