Useless "Replica Identity: NOTHING" noise from psql \d

On 2013-12-14 11:27:53 -0500, Tom Lane wrote:

In HEAD:

regression=# \d pg_depend
Table "pg_catalog.pg_depend"
Column | Type | Modifiers
-------------+---------+-----------
classid | oid | not null
objid | oid | not null
objsubid | integer | not null
refclassid | oid | not null
refobjid | oid | not null
refobjsubid | integer | not null
deptype | "char" | not null
Indexes:
"pg_depend_depender_index" btree (classid, objid, objsubid)
"pg_depend_reference_index" btree (refclassid, refobjid, refobjsubid)
Replica Identity: NOTHING

Where did that last line come from, and who thinks it's so important
that it should appear by default? It seems absolutely content-free
even if I were using whatever feature it refers to, since it is
(I presume) the default state.

Hm. Yes, that's slightly inellegant. It's shown because it's not
actually the normal default normal tables. Just for system tables. Maybe
we should just set it to default (in pg_class) for system tables as
well, and just change it in the relcache.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

On 2013-12-14 17:43:36 +0100, Andres Freund wrote:

On 2013-12-14 11:27:53 -0500, Tom Lane wrote:

In HEAD:

regression=# \d pg_depend
Table "pg_catalog.pg_depend"
Column | Type | Modifiers
-------------+---------+-----------
classid | oid | not null
objid | oid | not null
objsubid | integer | not null
refclassid | oid | not null
refobjid | oid | not null
refobjsubid | integer | not null
deptype | "char" | not null
Indexes:
"pg_depend_depender_index" btree (classid, objid, objsubid)
"pg_depend_reference_index" btree (refclassid, refobjid, refobjsubid)
Replica Identity: NOTHING

Where did that last line come from, and who thinks it's so important
that it should appear by default? It seems absolutely content-free
even if I were using whatever feature it refers to, since it is
(I presume) the default state.

Hm. Yes, that's slightly inellegant. It's shown because it's not
actually the normal default normal tables. Just for system tables. Maybe
we should just set it to default (in pg_class) for system tables as
well, and just change it in the relcache.

Hm. I don't like that choice much after thinking for a bit. Seems to
make querying the catalog unneccessarily complex.
How about making it conditional on the table's namespace instead? That
will do the wrong thing if somebody moves a table to pg_catalog and
configures a replica identity, but I think we can live with that, given
how many other things work strangely around that.

Patch attached.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Dec 16, 2013 at 7:25 AM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2013-12-14 17:43:36 +0100, Andres Freund wrote:

On 2013-12-14 11:27:53 -0500, Tom Lane wrote:

In HEAD:

regression=# \d pg_depend
Table "pg_catalog.pg_depend"
Column | Type | Modifiers
-------------+---------+-----------
classid | oid | not null
objid | oid | not null
objsubid | integer | not null
refclassid | oid | not null
refobjid | oid | not null
refobjsubid | integer | not null
deptype | "char" | not null
Indexes:
"pg_depend_depender_index" btree (classid, objid, objsubid)
"pg_depend_reference_index" btree (refclassid, refobjid, refobjsubid)
Replica Identity: NOTHING

Where did that last line come from, and who thinks it's so important
that it should appear by default? It seems absolutely content-free
even if I were using whatever feature it refers to, since it is
(I presume) the default state.

Hm. Yes, that's slightly inellegant. It's shown because it's not
actually the normal default normal tables. Just for system tables. Maybe
we should just set it to default (in pg_class) for system tables as
well, and just change it in the relcache.

Hm. I don't like that choice much after thinking for a bit. Seems to
make querying the catalog unneccessarily complex.
How about making it conditional on the table's namespace instead? That
will do the wrong thing if somebody moves a table to pg_catalog and
configures a replica identity, but I think we can live with that, given
how many other things work strangely around that.

Patch attached.

I vote for showing it only with +, but regardless of whether the value
matches the expected default. I'd keep the relkind test, though,
because I think I noticed that it currently shows up for indexes,
which is dumb.

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

On Tue, Dec 17, 2013 at 09:37:09AM -0500, Robert Haas wrote:

Patch attached.

I vote for showing it only with +, but regardless of whether the value
matches the expected default. I'd keep the relkind test, though,
because I think I noticed that it currently shows up for indexes,
which is dumb.

Is this the patch you had in mind? I kept the pg_catalog filter. Do we
want to always show the replica identity line for \d+?

test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | | plain | |
Replica Identity: full
Has OIDs: no

I used lower-case for the value, rather than all-caps.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce@momjian.us> writes:

Is this the patch you had in mind? I kept the pg_catalog filter. Do we
want to always show the replica identity line for \d+?

Doesn't seem like a great idea to remove the filter tests for replident
values and then not fix the display code to cope with those values.

I think this display code is well south of minimal acceptability anyhow:
if the column contains anything other than what it expects, it will print
a lie, meaning it's entirely not future-proof. I'd suggest a switch()
that prints "???" in the default: case.

regards, tom lane

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

On Sun, Mar 23, 2014 at 11:49:37AM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Is this the patch you had in mind? I kept the pg_catalog filter. Do we
want to always show the replica identity line for \d+?

Doesn't seem like a great idea to remove the filter tests for replident
values and then not fix the display code to cope with those values.

I think this display code is well south of minimal acceptability anyhow:
if the column contains anything other than what it expects, it will print
a lie, meaning it's entirely not future-proof. I'd suggest a switch()
that prints "???" in the default: case.

Oh, good points. I have updated the attached patch.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On 2014-03-22 23:47:57 -0400, Bruce Momjian wrote:

test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | | plain | |
Replica Identity: full
Has OIDs: no

I used lower-case for the value, rather than all-caps.

Why? CLUSTER, PRIMARY KEY, etc. are displayed all caps, and replica
identity is similarly set via ALTER TABLE ... REPLICA IDENITY?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

On Mon, Mar 24, 2014 at 05:06:25PM +0100, Andres Freund wrote:

On 2014-03-22 23:47:57 -0400, Bruce Momjian wrote:

test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | | plain | |
Replica Identity: full
Has OIDs: no

I used lower-case for the value, rather than all-caps.

Why? CLUSTER, PRIMARY KEY, etc. are displayed all caps, and replica
identity is similarly set via ALTER TABLE ... REPLICA IDENITY?

Oh, good points; I had not considered PRIMARY KEY. Updated patch
attached.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian wrote:

On Mon, Mar 24, 2014 at 05:06:25PM +0100, Andres Freund wrote:

On 2014-03-22 23:47:57 -0400, Bruce Momjian wrote:

test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | | plain | |
Replica Identity: full
Has OIDs: no

I used lower-case for the value, rather than all-caps.

Why? CLUSTER, PRIMARY KEY, etc. are displayed all caps, and replica
identity is similarly set via ALTER TABLE ... REPLICA IDENITY?

Oh, good points; I had not considered PRIMARY KEY. Updated patch
attached.

In the "INDEX" case, should the output mention specifically which index
is being considered?

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

On Mon, Mar 24, 2014 at 01:35:20PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

On Mon, Mar 24, 2014 at 05:06:25PM +0100, Andres Freund wrote:

On 2014-03-22 23:47:57 -0400, Bruce Momjian wrote:

test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | | plain | |
Replica Identity: full
Has OIDs: no

I used lower-case for the value, rather than all-caps.

Why? CLUSTER, PRIMARY KEY, etc. are displayed all caps, and replica
identity is similarly set via ALTER TABLE ... REPLICA IDENITY?

Oh, good points; I had not considered PRIMARY KEY. Updated patch
attached.

In the "INDEX" case, should the output mention specifically which index
is being considered?

Ah, good idea. Updated patch attached. The output is now:

test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | not null | plain | |
Indexes:
"test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY
"i_test2" btree (x)
--> Replica Identity: USING INDEX "test_pkey"
Has OIDs: no

However, now that I look at it, it seems redundant as REPLICA IDENTITY
is already marked on the actual index. Ideas?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Mon, Mar 24, 2014 at 09:07:07PM -0400, Bruce Momjian wrote:

In the "INDEX" case, should the output mention specifically which index
is being considered?

Ah, good idea. Updated patch attached. The output is now:

test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | not null | plain | |
Indexes:
"test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY
"i_test2" btree (x)
--> Replica Identity: USING INDEX "test_pkey"
Has OIDs: no

However, now that I look at it, it seems redundant as REPLICA IDENTITY
is already marked on the actual index. Ideas?

Hearing nothing, I have gone back to the previous patch that just marks
replica identity as USING INDEX; applied patch attached.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian wrote:

On Mon, Mar 24, 2014 at 09:07:07PM -0400, Bruce Momjian wrote:

In the "INDEX" case, should the output mention specifically which index
is being considered?

Ah, good idea. Updated patch attached. The output is now:

test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | not null | plain | |
Indexes:
"test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY
"i_test2" btree (x)
--> Replica Identity: USING INDEX "test_pkey"
Has OIDs: no

However, now that I look at it, it seems redundant as REPLICA IDENTITY
is already marked on the actual index. Ideas?

Hearing nothing, I have gone back to the previous patch that just marks
replica identity as USING INDEX; applied patch attached.

Not opposed to this, but it seems a bit strange; REPLICA IDENTITY is a
property of the table, not of any individual index. I think we should
lose the token in the "Indexes" section.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

On Wed, Mar 26, 2014 at 12:20:07PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

On Mon, Mar 24, 2014 at 09:07:07PM -0400, Bruce Momjian wrote:

In the "INDEX" case, should the output mention specifically which index
is being considered?

Ah, good idea. Updated patch attached. The output is now:

test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | not null | plain | |
Indexes:
"test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY
"i_test2" btree (x)
--> Replica Identity: USING INDEX "test_pkey"
Has OIDs: no

However, now that I look at it, it seems redundant as REPLICA IDENTITY
is already marked on the actual index. Ideas?

Hearing nothing, I have gone back to the previous patch that just marks
replica identity as USING INDEX; applied patch attached.

Not opposed to this, but it seems a bit strange; REPLICA IDENTITY is a
property of the table, not of any individual index. I think we should
lose the token in the "Indexes" section.

That is an interesting idea. It would mean that \d table would not show
anything about replica identity, because right now it does:

test=> \d test
Table "public.test"
Column | Type | Modifiers
--------+---------+-----------
x | integer | not null
Indexes:
"test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY

That seems logical. So under the new plan, \d would show:

test=> \d test
Table "public.test"
Column | Type | Modifiers
--------+---------+-----------
x | integer | not null
Indexes:
"test_pkey" PRIMARY KEY, btree (x)

and \d+ would show:

test=> \d+ test
Table "public.test"
Column | Type | Modifiers | Storage | Stats target | Description
--------+---------+-----------+---------+--------------+-------------
x | integer | not null | plain | |
Indexes:
"test_pkey" PRIMARY KEY, btree (x)
Replica Identity: USING INDEX "test_pkey"
Has OIDs: no

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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

Bruce Momjian wrote:

On Wed, Mar 26, 2014 at 12:20:07PM -0300, Alvaro Herrera wrote:

Not opposed to this, but it seems a bit strange; REPLICA IDENTITY is a
property of the table, not of any individual index. I think we should
lose the token in the "Indexes" section.

That is an interesting idea. It would mean that \d table would not show
anything about replica identity, because right now it does:

test=> \d test
Table "public.test"
Column | Type | Modifiers
--------+---------+-----------
x | integer | not null
Indexes:
"test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY

That seems logical.

Hmm. It seems to me that to make this more compact we could keep the
current token in the index line if it's INDEX, and not display the
Replica Identity: line at all; and if it's something other than index
and different from the default value, then print "Replica Identity" in
both \d and \d+.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

On Wed, Mar 26, 2014 at 12:53:32PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

On Wed, Mar 26, 2014 at 12:20:07PM -0300, Alvaro Herrera wrote:

Not opposed to this, but it seems a bit strange; REPLICA IDENTITY is a
property of the table, not of any individual index. I think we should
lose the token in the "Indexes" section.

That is an interesting idea. It would mean that \d table would not show
anything about replica identity, because right now it does:

test=> \d test
Table "public.test"
Column | Type | Modifiers
--------+---------+-----------
x | integer | not null
Indexes:
"test_pkey" PRIMARY KEY, btree (x) REPLICA IDENTITY

That seems logical.

Hmm. It seems to me that to make this more compact we could keep the
current token in the index line if it's INDEX, and not display the
Replica Identity: line at all; and if it's something other than index
and different from the default value, then print "Replica Identity" in
both \d and \d+.

OK. Tom's original complaint was about showing the default state in \d:

/messages/by-id/12303.1387038473@sss.pgh.pa.us

though that example was for an odd case where a system table didn't use
the default value.

The attached patch matches your suggestion. It is basically back to
what the code originally had, except it skips system tables, and shows
"???" for invalid values.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Re: Bruce Momjian 2014-03-26 <20140326161056.GA468@momjian.us>

The attached patch matches your suggestion. It is basically back to
what the code originally had, except it skips system tables, and shows
"???" for invalid values.

Fwiw, "make check-world" is currently broken:

******** build/contrib/test_decoding/regression.diffs ********
*** /tmp/buildd/postgresql-9.4-9.4~20140327.0501/build/../contrib/test_decoding/expected/ddl.out	Thu Mar 27 02:43:36 2014
--- /tmp/buildd/postgresql-9.4-9.4~20140327.0501/build/contrib/test_decoding/results/ddl.out	Thu Mar 27 05:14:02 2014
***************
*** 345,350 ****
--- 345,351 ----
   options  | text[]  |                                                                   | extended |              | 
  Indexes:
      "replication_metadata_pkey" PRIMARY KEY, btree (id)
+ Replica Identity: DEFAULT
  Has OIDs: no
  Options: user_catalog_table=true

***************
*** 360,365 ****
--- 361,367 ----
   options  | text[]  |                                                                   | extended |              | 
  Indexes:
      "replication_metadata_pkey" PRIMARY KEY, btree (id)
+ Replica Identity: DEFAULT
  Has OIDs: no

  INSERT INTO replication_metadata(relation, options)
***************
*** 374,379 ****
--- 376,382 ----
   options  | text[]  |                                                                   | extended |              | 
  Indexes:
      "replication_metadata_pkey" PRIMARY KEY, btree (id)
+ Replica Identity: DEFAULT
  Has OIDs: no
  Options: user_catalog_table=true

***************
*** 394,399 ****
--- 397,403 ----
   rewritemeornot | integer |                                                                   | plain    |              | 
  Indexes:
      "replication_metadata_pkey" PRIMARY KEY, btree (id)
+ Replica Identity: DEFAULT
  Has OIDs: no
  Options: user_catalog_table=false

Christoph
--
cb@df7cb.de | http://www.df7cb.de/

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

On Thu, Mar 27, 2014 at 10:02:20AM +0100, Christoph Berg wrote:

Re: Bruce Momjian 2014-03-26 <20140326161056.GA468@momjian.us>

The attached patch matches your suggestion. It is basically back to
what the code originally had, except it skips system tables, and shows
"???" for invalid values.

Fwiw, "make check-world" is currently broken:

Yes, the patch was partial to just show the code changes, to get
approval. Attached is the full patch.

I did some research of the regression database to see what was being
set:

SELECT relreplident, relkind, nspname, count(*)
FROM pg_class, pg_namespace
WHERE relkind IN ('m', 'r') AND
relnamespace = pg_namespace.oid AND
nspname != 'pg_catalog'
GROUP BY relreplident, nspname, relkind
ORDER BY 1, 2, 3;

relreplident | relkind | nspname | count
--------------+---------+--------------------+-------
d | m | mvschema | 1
d | m | public | 5
d | r | information_schema | 7
d | r | public | 205
d | r | testxmlschema | 3

It seems everything is default, which would not be displayed.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Re: Bruce Momjian 2014-03-27 <20140327131048.GA11694@momjian.us>

On Thu, Mar 27, 2014 at 10:02:20AM +0100, Christoph Berg wrote:

Re: Bruce Momjian 2014-03-26 <20140326161056.GA468@momjian.us>

The attached patch matches your suggestion. It is basically back to
what the code originally had, except it skips system tables, and shows
"???" for invalid values.

Fwiw, "make check-world" is currently broken:

Yes, the patch was partial to just show the code changes, to get
approval. Attached is the full patch.

I meant to say what's actually in git HEAD at the moment is broken.
The regression.diffs are without an extra patch. This prevents
building 9.4devel packages for apt.postgresql.org now.

Mit freundlichen Gr��en,
Christoph Berg
--
Senior Berater, Tel.: +49 (0)21 61 / 46 43-187
credativ GmbH, HRB M�nchengladbach 12080, USt-ID-Nummer: DE204566209
Hohenzollernstr. 133, 41061 M�nchengladbach
Gesch�ftsf�hrung: Dr. Michael Meskes, J�rg Folz, Sascha Heuer
pgp fingerprint: 5C48 FE61 57F4 9179 5970 87C6 4C5A 6BAB 12D2 A7AE

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

On Thu, Mar 27, 2014 at 02:41:40PM +0100, Christoph Berg wrote:

Re: Bruce Momjian 2014-03-27 <20140327131048.GA11694@momjian.us>

On Thu, Mar 27, 2014 at 10:02:20AM +0100, Christoph Berg wrote:

Re: Bruce Momjian 2014-03-26 <20140326161056.GA468@momjian.us>

The attached patch matches your suggestion. It is basically back to
what the code originally had, except it skips system tables, and shows
"???" for invalid values.

Fwiw, "make check-world" is currently broken:

Yes, the patch was partial to just show the code changes, to get
approval. Attached is the full patch.

I meant to say what's actually in git HEAD at the moment is broken.
The regression.diffs are without an extra patch. This prevents
building 9.4devel packages for apt.postgresql.org now.

Uh, I thought that might be what you were saying, but I am not seeing
any failures here. I don't see any platform-specific regression files
in the files I modified. Can you show me the failures?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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