Re: [HACKERS] PATCH: psql show index with type info

[Daniel Gustafsson]

> On 13 Sep 2017, at 01:24, Daniel Gustafsson  wrote:
> 
>> On 18 Apr 2017, at 05:13, Amos Bird  wrote:
>> 
>> Ah, thanks for the suggestions. I'll revise this patch soon :)
> 
> Have you had a chance to revise the patch to address the review comments such
> that the patch can move forward during this Commitfest?

Since the patch was Waiting for author without being updated during the
commitfest, it has been Returned with Feedback.

cheers ./daniel

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Daniel Gustafsson]

> On 18 Apr 2017, at 05:13, Amos Bird  wrote:
> 
> Ah, thanks for the suggestions. I'll revise this patch soon :)

Have you had a chance to revise the patch to address the review comments such
that the patch can move forward during this Commitfest?

cheers ./daniel


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Amos Bird]

Ah, thanks for the suggestions. I'll revise this patch soon :)

Fabien COELHO  writes:

>> Done.
>
> Ok. The file should be named "v2".
>
>> Would you like to be the reviewer?
>
> Dunno. At least I wanted to have a look at it!
>
> My 0.02€:
>
> I think that the improvement provided is worthwhile.
>
> Two questions: Why no documentation update? Why no non-regressions 
> tests?
>
> As far as the output is concerned, ISTM that "btree index", "hash index" 
> and so would sound nicer than "index: sub-type".
>
> I'm wondering about the sub-query implementation: Maybe an outer join 
> could bring the same result with a more relational & elegant query.
>
> The "gettext_noop" call does not seem to make sense: the point is to 
> translate the string, and there is no way to translate "index:  query>". The result of the query should be translated if this is what is 
> wanted, and that can only be by hand:
>
>CASE amname || ' index'
>  WHEN 'hash index' THEN '%s' ...
>  WHEN ...
>  ELSE amname || ' index' # untranslated...
>END
>
> gettext_noop('hash index') # get translation for 'hash index'
> ...



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Fabien COELHO]

Done.


Ok. The file should be named "v2".


Would you like to be the reviewer?


Dunno. At least I wanted to have a look at it!

My 0.02€:

I think that the improvement provided is worthwhile.

Two questions: Why no documentation update? Why no non-regressions 
tests?


As far as the output is concerned, ISTM that "btree index", "hash index" 
and so would sound nicer than "index: sub-type".


I'm wondering about the sub-query implementation: Maybe an outer join 
could bring the same result with a more relational & elegant query.


The "gettext_noop" call does not seem to make sense: the point is to 
translate the string, and there is no way to translate "index: query>". The result of the query should be translated if this is what is 
wanted, and that can only be by hand:


  CASE amname || ' index'
WHEN 'hash index' THEN '%s' ...
WHEN ...
ELSE amname || ' index' # untranslated...
  END

   gettext_noop('hash index') # get translation for 'hash index'
   ...

--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Amos Bird]

Done. Would you like to be the reviewer? Thanks! diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 0f9f497..a6dc599 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3284,7 +3284,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
 	  " WHEN " CppAsString2(RELKIND_RELATION) " THEN '%s'"
 	  " WHEN " CppAsString2(RELKIND_VIEW) " THEN '%s'"
 	  " WHEN " CppAsString2(RELKIND_MATVIEW) " THEN '%s'"
-	  " WHEN " CppAsString2(RELKIND_INDEX) " THEN '%s'"
+	  " WHEN " CppAsString2(RELKIND_INDEX) " THEN %s"
 	  " WHEN " CppAsString2(RELKIND_SEQUENCE) " THEN '%s'"
 	  " WHEN 's' THEN '%s'"
 	  " WHEN " CppAsString2(RELKIND_FOREIGN_TABLE) " THEN '%s'"
@@ -3296,7 +3296,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
 	  gettext_noop("table"),
 	  gettext_noop("view"),
 	  gettext_noop("materialized view"),
-	  gettext_noop("index"),
+	  gettext_noop("'index: '||(select amname from pg_am a where a.oid = c.relam)"),
 	  gettext_noop("sequence"),
 	  gettext_noop("special"),
 	  gettext_noop("foreign table"),
.

Fabien COELHO  writes:

>> I'm not sure where to add documentations about this patch or if needed one. 
>> Please help
>> me if you think this patch is useful.
>
> This patch does not apply anymore. Are you planning to update it?

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Fabien COELHO]

I'm not sure where to add documentations about this patch or if needed one. 
Please help
me if you think this patch is useful.


This patch does not apply anymore. Are you planning to update it?

--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Mar 6, 2017 at 9:30 AM, Stephen Frost  wrote:
> > * Amos Bird (amosb...@gmail.com) wrote:
> >> Well, the prefix is used to differentiate other \d commands, like
> >> this,
> >
> > Ah, ok, fair enough.
> >
> > Should we consider differentiating different table types also?  I
> > suppose those are primairly just logged and unlogged, but I could see
> > that being useful information to know when doing a \dt.  Not a big deal
> > either way though, and this patch stands on its own certainly.
> 
> Logged vs. unlogged isn't the same thing as identifying the access
> method.  Anything with storage can come in logged, unlogged, and
> temporary varieties, but an access method is essentially a way of
> saying that one object has a different physical layout than another.
> Right now there is, indeed, only one heap access method, but some of
> my colleagues and I are scheming over how to change that.

Fair enough, sounds like a good reason to add an Access Method column to
me.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: psql show index with type info

[Robert Haas]

On Mon, Mar 6, 2017 at 9:30 AM, Stephen Frost  wrote:
> * Amos Bird (amosb...@gmail.com) wrote:
>> Well, the prefix is used to differentiate other \d commands, like
>> this,
>
> Ah, ok, fair enough.
>
> Should we consider differentiating different table types also?  I
> suppose those are primairly just logged and unlogged, but I could see
> that being useful information to know when doing a \dt.  Not a big deal
> either way though, and this patch stands on its own certainly.

Logged vs. unlogged isn't the same thing as identifying the access
method.  Anything with storage can come in logged, unlogged, and
temporary varieties, but an access method is essentially a way of
saying that one object has a different physical layout than another.
Right now there is, indeed, only one heap access method, but some of
my colleagues and I are scheming over how to change that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Stephen Frost]

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 3/8/17 08:30, Stephen Frost wrote:
> > Right, I don't think having an extra column which is going to be NULL a
> > large amount of the time is good.
> 
> Note that \di already has a column "Table" that is null for something
> that is not an index.  So I don't think this argument is very strong.

That's an interesting point.

I think what I find most odd about all of this is that \dti and \dit
work at all, and give a different set of columns from \dt.  We don't
document that combining those works in \?, as far as I can see, and
other combinations don't work, just this.

In any case, I won't push very hard on this, it's useful information to
include and we should do so.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: psql show index with type info

[Stephen Frost]

Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 3/8/17 04:11, Ashutosh Bapat wrote:
> > The header for this table is "list of relations", so type gets
> > associated with relations indicated type of relation. btree: gin as a
> > type of relation doesn't sound really great. Instead we might want to
> > add another column "access method" and specify the access method used
> > for that relation.
> 
> I like that.

When it will be NULL for all of the regular tables..?

I'd rather have the one Type column that just included the access method
when there is one to include.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: psql show index with type info

[Peter Eisentraut]

On 3/8/17 08:30, Stephen Frost wrote:
> Right, I don't think having an extra column which is going to be NULL a
> large amount of the time is good.

Note that \di already has a column "Table" that is null for something
that is not an index.  So I don't think this argument is very strong.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Peter Eisentraut]

On 3/8/17 04:11, Ashutosh Bapat wrote:
> The header for this table is "list of relations", so type gets
> associated with relations indicated type of relation. btree: gin as a
> type of relation doesn't sound really great. Instead we might want to
> add another column "access method" and specify the access method used
> for that relation.

I like that.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Ashutosh Bapat]

On Wed, Mar 8, 2017 at 7:00 PM, Stephen Frost  wrote:
> Ashutosh,
>
> * Ashutosh Bapat (ashutosh.ba...@enterprisedb.com) wrote:
>> On Mon, Mar 6, 2017 at 7:54 PM, Amos Bird  wrote:
>> > Well, the prefix is used to differentiate other \d commands, like
>> > this,
>> >
>> > amos=# \ditv
>> >   List of relations
>> >  Schema |Name| Type | Owner |  Table
>> > ++--+---+-
>> >  public | i  | table| amos  |
>> >  public | ii | index: gist  | amos  | i
>> >  public | j  | table| amos  |
>> >  public | jj | index: gin   | amos  | i
>> >  public | jp | index: btree | amos  | i
>> >  public | js | index: brin  | amos  | i
>> >  public | numbers| table| amos  |
>> >  public | numbers_mod2   | index: gin   | amos  | numbers
>> >  public | numbers_mod2_btree | index: btree | amos  | numbers
>> >  public | ts | table| amos  |
>> > (10 rows)
>>
>> The header for this table is "list of relations", so type gets
>> associated with relations indicated type of relation. btree: gin as a
>> type of relation doesn't sound really great.
>
> The type is 'index', we're just adding a sub-type here to clarify the
> kind of index it is.
>
>> Instead we might want to
>> add another column "access method" and specify the access method used
>> for that relation. But then only indexes seem to have access methods
>> per pg_class.h.
>
> Right, I don't think having an extra column which is going to be NULL a
> large amount of the time is good.  The approach taken by Amos seems like
> a good one to me, to have the type still be 'index' but with a sub-type
> indicating the access method.

Ok. Seems like a good trade-off.


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Stephen Frost]

Ashutosh,

* Ashutosh Bapat (ashutosh.ba...@enterprisedb.com) wrote:
> On Mon, Mar 6, 2017 at 7:54 PM, Amos Bird  wrote:
> > Well, the prefix is used to differentiate other \d commands, like
> > this,
> >
> > amos=# \ditv
> >   List of relations
> >  Schema |Name| Type | Owner |  Table
> > ++--+---+-
> >  public | i  | table| amos  |
> >  public | ii | index: gist  | amos  | i
> >  public | j  | table| amos  |
> >  public | jj | index: gin   | amos  | i
> >  public | jp | index: btree | amos  | i
> >  public | js | index: brin  | amos  | i
> >  public | numbers| table| amos  |
> >  public | numbers_mod2   | index: gin   | amos  | numbers
> >  public | numbers_mod2_btree | index: btree | amos  | numbers
> >  public | ts | table| amos  |
> > (10 rows)
> 
> The header for this table is "list of relations", so type gets
> associated with relations indicated type of relation. btree: gin as a
> type of relation doesn't sound really great.

The type is 'index', we're just adding a sub-type here to clarify the
kind of index it is.

> Instead we might want to
> add another column "access method" and specify the access method used
> for that relation. But then only indexes seem to have access methods
> per pg_class.h.

Right, I don't think having an extra column which is going to be NULL a
large amount of the time is good.  The approach taken by Amos seems like
a good one to me, to have the type still be 'index' but with a sub-type
indicating the access method.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: psql show index with type info

[Ashutosh Bapat]

On Mon, Mar 6, 2017 at 7:54 PM, Amos Bird  wrote:
>
> Hello Stephen,
>
> Well, the prefix is used to differentiate other \d commands, like
> this,
>
> amos=# \ditv
>   List of relations
>  Schema |Name| Type | Owner |  Table
> ++--+---+-
>  public | i  | table| amos  |
>  public | ii | index: gist  | amos  | i
>  public | j  | table| amos  |
>  public | jj | index: gin   | amos  | i
>  public | jp | index: btree | amos  | i
>  public | js | index: brin  | amos  | i
>  public | numbers| table| amos  |
>  public | numbers_mod2   | index: gin   | amos  | numbers
>  public | numbers_mod2_btree | index: btree | amos  | numbers
>  public | ts | table| amos  |
> (10 rows)
>

The header for this table is "list of relations", so type gets
associated with relations indicated type of relation. btree: gin as a
type of relation doesn't sound really great. Instead we might want to
add another column "access method" and specify the access method used
for that relation. But then only indexes seem to have access methods
per pg_class.h.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Amos Bird]

Yeah, I'm thinking about that too. Here is a full list of the original
type values,

"Schema"
"Name"
"table"
"view"
"materialized view"
"index"
"sequence"
"special"
"foreign table"
"table"

What else do you think will benefit from extra type information?

regards,
Amos

Stephen Frost  writes:

> Amos,
>
> * Amos Bird (amosb...@gmail.com) wrote:
>> Well, the prefix is used to differentiate other \d commands, like
>> this,
>
> Ah, ok, fair enough.
>
> Should we consider differentiating different table types also?  I
> suppose those are primairly just logged and unlogged, but I could see
> that being useful information to know when doing a \dt.  Not a big deal
> either way though, and this patch stands on its own certainly.
>
> Thanks!
>
> Stephen



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Stephen Frost]

Amos,

* Amos Bird (amosb...@gmail.com) wrote:
> Well, the prefix is used to differentiate other \d commands, like
> this,

Ah, ok, fair enough.

Should we consider differentiating different table types also?  I
suppose those are primairly just logged and unlogged, but I could see
that being useful information to know when doing a \dt.  Not a big deal
either way though, and this patch stands on its own certainly.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: psql show index with type info

[Amos Bird]

Hello Stephen,

Well, the prefix is used to differentiate other \d commands, like
this,

amos=# \ditv
  List of relations
 Schema |Name| Type | Owner |  Table
++--+---+-
 public | i  | table| amos  |
 public | ii | index: gist  | amos  | i
 public | j  | table| amos  |
 public | jj | index: gin   | amos  | i
 public | jp | index: btree | amos  | i
 public | js | index: brin  | amos  | i
 public | numbers| table| amos  |
 public | numbers_mod2   | index: gin   | amos  | numbers
 public | numbers_mod2_btree | index: btree | amos  | numbers
 public | ts | table| amos  |
(10 rows)

regards,
Amos

Stephen Frost  writes:

> Greetings,
>
> * Amos Bird (amosb...@gmail.com) wrote:
>> psql currently supports \di+ to view indexes,
>> 
>>   List of relations
>>  Schema |Name| Type  | Owner |  Table  |  Size  | Description
>> ++---+---+-++-
>>  public | ii | index | amos  | i   | 131 MB |
>>  public | jj | index | amos  | i   | 12 MB  |
>>  public | kk | index | amos  | i   | 456 kB |
>>  public | numbers_mod2   | index | amos  | numbers | 10 MB  |
>>  public | numbers_mod2_btree | index | amos  | numbers | 214 MB |
>> (5 rows)
>> 
>> The co
>> lumn "Type" is kinda useless (all equals to index). Representing
>> the actual index type will be more interesting,
>
> Agreed.
>
>>  Schema |Name| Type | Owner |  Table  |  Size  | 
>> Description
>> ++--+---+-++-
>>  public | ii | index: gist  | amos  | i   | 131 MB |
>>  public | jj | index: gin   | amos  | i   | 12 MB  |
>>  public | kk | index: btree | amos  | i   | 456 kB |
>>  public | numbers_mod2   | index: gin   | amos  | numbers | 10 MB  |
>>  public | numbers_mod2_btree | index: btree | amos  | numbers | 214 MB |
>> (5 rows)
>> 
>> I'm not sure where to add documentations about this patch or if needed one. 
>> Please help
>> me if you think this patch is useful.
>
> I'm not sure why it's useful to keep the 'index:'?  I would suggest we
> just drop that and keep only the actual index type (gist, gin, etc).
>
> Thanks!
>
> Stephen



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] PATCH: psql show index with type info

[Stephen Frost]

Greetings,

* Amos Bird (amosb...@gmail.com) wrote:
> psql currently supports \di+ to view indexes,
> 
>   List of relations
>  Schema |Name| Type  | Owner |  Table  |  Size  | Description
> ++---+---+-++-
>  public | ii | index | amos  | i   | 131 MB |
>  public | jj | index | amos  | i   | 12 MB  |
>  public | kk | index | amos  | i   | 456 kB |
>  public | numbers_mod2   | index | amos  | numbers | 10 MB  |
>  public | numbers_mod2_btree | index | amos  | numbers | 214 MB |
> (5 rows)
> 
> The co
> lumn "Type" is kinda useless (all equals to index). Representing
> the actual index type will be more interesting,

Agreed.

>  Schema |Name| Type | Owner |  Table  |  Size  | 
> Description
> ++--+---+-++-
>  public | ii | index: gist  | amos  | i   | 131 MB |
>  public | jj | index: gin   | amos  | i   | 12 MB  |
>  public | kk | index: btree | amos  | i   | 456 kB |
>  public | numbers_mod2   | index: gin   | amos  | numbers | 10 MB  |
>  public | numbers_mod2_btree | index: btree | amos  | numbers | 214 MB |
> (5 rows)
> 
> I'm not sure where to add documentations about this patch or if needed one. 
> Please help
> me if you think this patch is useful.

I'm not sure why it's useful to keep the 'index:'?  I would suggest we
just drop that and keep only the actual index type (gist, gin, etc).

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] PATCH: psql show index with type info

[Amos Bird]

psql currently supports \di+ to view indexes,

  List of relations
 Schema |Name| Type  | Owner |  Table  |  Size  | Description
++---+---+-++-
 public | ii | index | amos  | i   | 131 MB |
 public | jj | index | amos  | i   | 12 MB  |
 public | kk | index | amos  | i   | 456 kB |
 public | numbers_mod2   | index | amos  | numbers | 10 MB  |
 public | numbers_mod2_btree | index | amos  | numbers | 214 MB |
(5 rows)

The co
lumn "Type" is kinda useless (all equals to index). Representing
the actual index type will be more interesting,

 Schema |Name| Type | Owner |  Table  |  Size  | 
Description
++--+---+-++-
 public | ii | index: gist  | amos  | i   | 131 MB |
 public | jj | index: gin   | amos  | i   | 12 MB  |
 public | kk | index: btree | amos  | i   | 456 kB |
 public | numbers_mod2   | index: gin   | amos  | numbers | 10 MB  |
 public | numbers_mod2_btree | index: btree | amos  | numbers | 214 MB |
(5 rows)

I'm not sure where to add documentations about this patch or if needed one. 
Please help
me if you think this patch is useful.

Best regards,
Amos

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index e2e4cbc..ac27662 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3169,7 +3169,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
  " WHEN 'r' THEN '%s'"
  " WHEN 'v' THEN '%s'"
  " WHEN 'm' THEN '%s'"
- " WHEN 'i' THEN '%s'"
+ " WHEN 'i' THEN %s"
  " WHEN 'S' THEN '%s'"
  " WHEN 's' THEN '%s'"
  " WHEN 'f' THEN '%s'"
@@ -3181,7 +3181,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
  gettext_noop("table"),
  gettext_noop("view"),
  gettext_noop("materialized view"),
- gettext_noop("index"),
+ gettext_noop("'index: '||(select amname from pg_am a where a.oid = c.relam)"),
  gettext_noop("sequence"),
  gettext_noop("special"),
  gettext_noop("foreign table"),

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers