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

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

Hi,

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, 2, true);
pg_get_indexdef
-----------------
f2 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 :-(

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

Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com

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. +