[PATCH v1] Show whether tables are logged in \dt+
Folks,
I noticed that there wasn't a bulk way to see table logged-ness in
psql, so I made it part of \dt+.
What say?
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
v1-0001-Show-whether-tables-are-logged-in-dt.patchtext/x-diff; charset=us-asciiDownload
From 518efc003730c441663aae42c18d16f0908bd55d Mon Sep 17 00:00:00 2001
From: David Fetter <david.fetter@onelogin.com>
Date: Mon, 22 Apr 2019 17:50:48 -0700
Subject: [PATCH v1] Show whether tables are logged in \dt+
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
\d would show this for individual tables, but there wasn't an
overarching view of all tables. Now, there is.
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index ee00c5da08..5ef567c123 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3631,7 +3631,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
- static const bool translate_columns[] = {false, false, true, false, false, false, false};
+ static const bool translate_columns[] = {false, false, true, false, false, false, false, false};
/* If tabtypes is empty, we default to \dtvmsE (but see also command.c) */
if (!(showTables || showIndexes || showViews || showMatViews || showSeq || showForeign))
@@ -3695,6 +3695,11 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
appendPQExpBuffer(&buf,
",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
gettext_noop("Description"));
+
+ if (pset.sversion >= 91000)
+ appendPQExpBuffer(&buf,
+ ",\n c.relpersistence <> 'u' as \"%s\"",
+ gettext_noop("Logged"));
}
appendPQExpBufferStr(&buf,
--------------2.20.1--
Hello David,
I noticed that there wasn't a bulk way to see table logged-ness in psql,
so I made it part of \dt+.
Applies, compiles, works for me.
ISTM That temporary-ness is not shown either. Maybe the persistence column
should be shown as is?
Also I'd suggest that the column should be displayed before the
"description" column to keep the length-varying one last?
What say?
Tests? Doc?
--
Fabien.
On Tue, Apr 23, 2019 at 07:03:58AM +0200, Fabien COELHO wrote:
Hello David,
I noticed that there wasn't a bulk way to see table logged-ness in psql,
so I made it part of \dt+.Applies, compiles, works for me.
ISTM That temporary-ness is not shown either. Maybe the persistence column
should be shown as is?
Temporariness added, but not raw.
Also I'd suggest that the column should be displayed before the
"description" column to keep the length-varying one last?
Done.
What say?
Tests?
Included, but they're not stable for temp tables. I'm a little stumped
as to how to either stabilize them or test some other way.
Doc?
What further documentation does it need?
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
v2-0001-Show-table-persistence-in-dt.patchtext/x-diff; charset=us-asciiDownload
From 9e82160c2fe2f554e3417b3fbff147bc6024e3fe Mon Sep 17 00:00:00 2001
From: David Fetter <david.fetter@onelogin.com>
Date: Mon, 22 Apr 2019 17:50:48 -0700
Subject: [PATCH v2] Show table persistence in \dt+
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
\d would show this for individual tables, but there wasn't an
overarching view of all tables. Now, there is.
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 5ef567c123..02e7eb0862 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3679,6 +3679,13 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
if (verbose)
{
+ /*
+ * Show whether the table is permanent, temporary, or unlogged.
+ */
+ if (pset.sversion >= 91000)
+ appendPQExpBuffer(&buf,
+ ",\n case c.relpersistence when 'p' then 'permanent' when 't' then 'temporary' when 'u' then 'unlogged' else 'unknown' end as \"%s\"",
+ gettext_noop("Persistence"));
/*
* As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
* size of a table, including FSM, VM and TOAST tables.
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 35856bffdd..f4f468804f 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -4729,3 +4729,16 @@ drop schema testpart;
set search_path to default;
set role to default;
drop role testrole_partitioning;
+create table foo(id integer);
+create temp table tfoo(id integer);
+create unlogged table ufoo(id integer);
+\dt+ *.*foo
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Size | Description
+-----------+------+-------+---------+-------------+---------+-------------
+ pg_temp_3 | tfoo | table | shackle | temporary | 0 bytes |
+ public | foo | table | shackle | permanent | 0 bytes |
+ public | ufoo | table | shackle | unlogged | 0 bytes |
+(3 rows)
+
+drop table foo, tfoo, ufoo;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 78f4b5d7d5..c9ad6ffd9c 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1115,3 +1115,10 @@ set search_path to default;
set role to default;
drop role testrole_partitioning;
+
+set search_path = public, pg_temp;
+create table foo(id integer);
+create temp table tfoo(id integer);
+create unlogged table ufoo(id integer);
+\dt+ *.*foo
+drop table foo, tfoo, ufoo;
--------------2.20.1--
Hello David,
I noticed that there wasn't a bulk way to see table logged-ness in psql,
so I made it part of \dt+.Applies, compiles, works for me.
ISTM That temporary-ness is not shown either. Maybe the persistence column
should be shown as is?Temporariness added, but not raw.
Ok, it is better like this way.
Tests?
Included, but they're not stable for temp tables. I'm a little stumped
as to how to either stabilize them or test some other way.
Hmmm. First there is the username which appears, so there should be a
dedicated user for the test.
I'm unsure how to work around the temporary schema number, which is
undeterministic with parallel execution it. I'm afraid the only viable
approach is not to show temporary tables, too bad:-(
Doc?
What further documentation does it need?
Indeed, there is no precise doc, so nothing to update :-)/:-(
Maybe you could consider adding a case for prior 9.1 version, something
like:
... case c.relistemp then 'temporary' else 'permanent' end as ...
--
Fabien.
On Wed, 24 Apr 2019 at 10:30, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Hello David,
I noticed that there wasn't a bulk way to see table logged-ness in psql,
so I made it part of \dt+.Applies, compiles, works for me.
ISTM That temporary-ness is not shown either. Maybe the persistence column
should be shown as is?Temporariness added, but not raw.
Ok, it is better like this way.
Tests?
Included, but they're not stable for temp tables. I'm a little stumped
as to how to either stabilize them or test some other way.Hmmm. First there is the username which appears, so there should be a
dedicated user for the test.I'm unsure how to work around the temporary schema number, which is
undeterministic with parallel execution it. I'm afraid the only viable
approach is not to show temporary tables, too bad:-(Doc?
What further documentation does it need?
Indeed, there is no precise doc, so nothing to update :-)/:-(
Maybe you could consider adding a case for prior 9.1 version, something
like:
... case c.relistemp then 'temporary' else 'permanent' end as ...
I was reviewing this patch and found a bug,
create table t (i int);
create index idx on t(i);
\di+
psql: print.c:3452: printQuery: Assertion `opt->translate_columns ==
((void *)0) || opt->n_translate_columns >= cont.ncolumns' failed.
--
Regards,
Rafia Sabih
On Fri, 26 Apr 2019 at 14:49, Rafia Sabih <rafia.pghackers@gmail.com> wrote:
On Wed, 24 Apr 2019 at 10:30, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Hello David,
I noticed that there wasn't a bulk way to see table logged-ness in psql,
so I made it part of \dt+.Applies, compiles, works for me.
ISTM That temporary-ness is not shown either. Maybe the persistence column
should be shown as is?Temporariness added, but not raw.
Ok, it is better like this way.
Tests?
Included, but they're not stable for temp tables. I'm a little stumped
as to how to either stabilize them or test some other way.Hmmm. First there is the username which appears, so there should be a
dedicated user for the test.I'm unsure how to work around the temporary schema number, which is
undeterministic with parallel execution it. I'm afraid the only viable
approach is not to show temporary tables, too bad:-(Doc?
What further documentation does it need?
Indeed, there is no precise doc, so nothing to update :-)/:-(
Maybe you could consider adding a case for prior 9.1 version, something
like:
... case c.relistemp then 'temporary' else 'permanent' end as ...I was reviewing this patch and found a bug,
create table t (i int);
create index idx on t(i);
\di+
psql: print.c:3452: printQuery: Assertion `opt->translate_columns ==
((void *)0) || opt->n_translate_columns >= cont.ncolumns' failed.
Looking into this further, apparently the position of
if (verbose)
{
+ /*
+ * Show whether the table is permanent, temporary, or unlogged.
+ */
+ if (pset.sversion >= 91000)
+ appendPQExpBuffer(&buf,
+ ",\n case c.relpersistence when 'p' then 'permanent' when 't'
then 'temporary' when 'u' then 'unlogged' else 'unknown' end as
\"%s\"",
+ gettext_noop("Persistence"));is not right, it is being called for indexes with verbose option also.
There should be an extra check for it being not called for index case.
Something like,
if (verbose)
{
/*
* Show whether the table is permanent, temporary, or unlogged.
*/
if (!showIndexes)
if (pset.sversion >= 91000)
appendPQExpBuffer(&buf,
",\n case c.relpersistence when 'p' then 'permanent' when 't' then
'temporary' when 'u' then 'unlogged' else 'unknown' end as \"%s\"",
gettext_noop("Persistence"));
Not sure, how do modify it in a more neat way.
--
Regards,
Rafia Sabih
On Fri, Apr 26, 2019 at 04:22:18PM +0200, Rafia Sabih wrote:
On Fri, 26 Apr 2019 at 14:49, Rafia Sabih <rafia.pghackers@gmail.com> wrote:
On Wed, 24 Apr 2019 at 10:30, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Hello David,
I noticed that there wasn't a bulk way to see table logged-ness in psql,
so I made it part of \dt+.Applies, compiles, works for me.
ISTM That temporary-ness is not shown either. Maybe the persistence column
should be shown as is?Temporariness added, but not raw.
Ok, it is better like this way.
Tests?
Included, but they're not stable for temp tables. I'm a little stumped
as to how to either stabilize them or test some other way.Hmmm. First there is the username which appears, so there should be a
dedicated user for the test.I'm unsure how to work around the temporary schema number, which is
undeterministic with parallel execution it. I'm afraid the only viable
approach is not to show temporary tables, too bad:-(Doc?
What further documentation does it need?
Indeed, there is no precise doc, so nothing to update :-)/:-(
Maybe you could consider adding a case for prior 9.1 version, something
like:
... case c.relistemp then 'temporary' else 'permanent' end as ...I was reviewing this patch and found a bug,
create table t (i int);
create index idx on t(i);
\di+
psql: print.c:3452: printQuery: Assertion `opt->translate_columns ==
((void *)0) || opt->n_translate_columns >= cont.ncolumns' failed.Looking into this further, apparently the position of
if (verbose) { + /* + * Show whether the table is permanent, temporary, or unlogged. + */ + if (pset.sversion >= 91000) + appendPQExpBuffer(&buf, + ",\n case c.relpersistence when 'p' then 'permanent' when 't' then 'temporary' when 'u' then 'unlogged' else 'unknown' end as \"%s\"", + gettext_noop("Persistence"));is not right, it is being called for indexes with verbose option also.
There should be an extra check for it being not called for index case.
Something like,
if (verbose)
{
/*
* Show whether the table is permanent, temporary, or unlogged.
*/
if (!showIndexes)
if (pset.sversion >= 91000)
appendPQExpBuffer(&buf,
",\n case c.relpersistence when 'p' then 'permanent' when 't' then
'temporary' when 'u' then 'unlogged' else 'unknown' end as \"%s\"",
gettext_noop("Persistence"));Not sure, how do modify it in a more neat way.
I suspect that as this may get a little messier, but I've made it
fairly neat short of a major refactor.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
v3-0001-Show-detailed-table-persistence-in-dt.patchtext/x-diff; charset=us-asciiDownload
From c5bd317b9904fe017afa5489b11a3cbae6e80f3b Mon Sep 17 00:00:00 2001
From: David Fetter <david.fetter@onelogin.com>
Date: Mon, 22 Apr 2019 17:50:48 -0700
Subject: [PATCH v3] Show detailed table persistence in \dt+
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
\d would show this for individual tables, but there wasn't an
overarching view of all tables. Now, there is.
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index ee00c5da08..620e7d69bf 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3631,7 +3631,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
- static const bool translate_columns[] = {false, false, true, false, false, false, false};
+ static const bool translate_columns[] = {false, false, true, false, false, false, false, false};
/* If tabtypes is empty, we default to \dtvmsE (but see also command.c) */
if (!(showTables || showIndexes || showViews || showMatViews || showSeq || showForeign))
@@ -3680,17 +3680,32 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
if (verbose)
{
/*
- * As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
- * size of a table, including FSM, VM and TOAST tables.
+ * Show whether a table is permanent, temporary, or unlogged.
+ * Indexes are not, as of this writing, tables.
*/
- if (pset.sversion >= 90000)
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
- gettext_noop("Size"));
- else if (pset.sversion >= 80100)
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_relation_size(c.oid)) as \"%s\"",
- gettext_noop("Size"));
+ if (!showIndexes)
+ {
+ if (pset.sversion >= 91000)
+ appendPQExpBuffer(&buf,
+ ",\n case c.relpersistence when 'p' then 'permanent' when 't' then 'temporary' when 'u' then 'unlogged' else 'unknown' end as \"%s\"",
+ gettext_noop("Persistence"));
+ else
+ appendPQExpBuffer(&buf,
+ ",\n case when c.relistemp then 'temporary' else 'permanent' end as \"%s\"",
+ gettext_noop("Persistence"));
+ }
+ /*
+ * As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
+ * size of a table, including FSM, VM and TOAST tables.
+ */
+ if (pset.sversion >= 90000)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
+ else if (pset.sversion >= 80100)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_relation_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
appendPQExpBuffer(&buf,
",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 35856bffdd..f4f468804f 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -4729,3 +4729,16 @@ drop schema testpart;
set search_path to default;
set role to default;
drop role testrole_partitioning;
+create table foo(id integer);
+create temp table tfoo(id integer);
+create unlogged table ufoo(id integer);
+\dt+ *.*foo
+ List of relations
+ Schema | Name | Type | Owner | Persistence | Size | Description
+-----------+------+-------+---------+-------------+---------+-------------
+ pg_temp_3 | tfoo | table | shackle | temporary | 0 bytes |
+ public | foo | table | shackle | permanent | 0 bytes |
+ public | ufoo | table | shackle | unlogged | 0 bytes |
+(3 rows)
+
+drop table foo, tfoo, ufoo;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 78f4b5d7d5..c9ad6ffd9c 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1115,3 +1115,10 @@ set search_path to default;
set role to default;
drop role testrole_partitioning;
+
+set search_path = public, pg_temp;
+create table foo(id integer);
+create temp table tfoo(id integer);
+create unlogged table ufoo(id integer);
+\dt+ *.*foo
+drop table foo, tfoo, ufoo;
--------------2.20.1--
Hello David,
Patch v3 applies, but compiles for me with a warning because the
indentation of the following size block has been changed:
describe.c: In function οΏ½listTablesοΏ½:
describe.c:3705:7: warning: this οΏ½ifοΏ½ clause does not guard...
[-Wmisleading-indentation]
else if (pset.sversion >= 80100)
^~
describe.c:3710:3: note: ...this statement, but the latter is misleadingly
indented as if it were guarded by the οΏ½ifοΏ½
appendPQExpBuffer(&buf,
^~~~~~~~~~~~~~~~~
Make check fails because of my temp schema was numbered 4 instead of 3,
and I'm "fabien" rather than "shackle".
Included, but they're not stable for temp tables. I'm a little stumped
as to how to either stabilize them or test some other way.Hmmm. First there is the username which appears, so there should be a
dedicated user for the test.I'm unsure how to work around the temporary schema number, which is
undeterministic with parallel execution it. I'm afraid the only viable
approach is not to show temporary tables, too bad:-(
The tests have not been fixed.
I think that they need a dedicated user to replace "shackle", and I'm
afraid that there temporary test schema instability cannot be easily fixed
at the "psql" level, but would require some kind of TAP tests instead if
it is to be checked. In the short term, do not.
I checked that the \di+ works, though. I've played with temporary views
and \dv as well.
I discovered that you cannot have temporary unlogged objects, nor
temporary or unlogged materialized views. Intuitively I'd have thought
that these features would be orthogonal, but they are not. Also I created
an unlogged table with a SERIAL which created a sequence. The table is
unlogged but the sequence is permanent, which is probably ok.
I only have packages down to pg 9.3, so I could not test prior 9.1. By
looking at the online documentation, is seems that relistemp appears in pg
8.4, so the corresponding extraction should be guarded by this version.
Before that, temporary objects existed but were identified indirectly,
possibly because they were stored in a temporary schema. I suggest not to
try to address cases prior 8.4.
--
Fabien.
On Sat, Apr 27, 2019 at 09:19:57AM +0200, Fabien COELHO wrote:
Hello David,
Patch v3 applies, but compiles for me with a warning because the indentation
of the following size block has been changed:describe.c: In function ‘listTables’:
describe.c:3705:7: warning: this ‘if’ clause does not guard...
[-Wmisleading-indentation]
else if (pset.sversion >= 80100)
^~
describe.c:3710:3: note: ...this statement, but the latter is misleadingly
indented as if it were guarded by the ‘if’
appendPQExpBuffer(&buf,
^~~~~~~~~~~~~~~~~
Fixed.
Make check fails because of my temp schema was numbered 4 instead of 3, and
I'm "fabien" rather than "shackle".
I think the way forward is to test this with TAP rather than the
fixed-string method.
Included, but they're not stable for temp tables. I'm a little stumped
as to how to either stabilize them or test some other way.Hmmm. First there is the username which appears, so there should be a
dedicated user for the test.I'm unsure how to work around the temporary schema number, which is
undeterministic with parallel execution it. I'm afraid the only viable
approach is not to show temporary tables, too bad:-(The tests have not been fixed.
I think that they need a dedicated user to replace "shackle", and I'm afraid
that there temporary test schema instability cannot be easily fixed at the
"psql" level, but would require some kind of TAP tests instead if it is to
be checked. In the short term, do not.
Checks removed while I figure out a new TAP test.
I checked that the \di+ works, though. I've played with temporary views and
\dv as well.
Great!
I discovered that you cannot have temporary unlogged objects, nor
temporary or unlogged materialized views. Intuitively I'd have
thought that these features would be orthogonal, but they are not.
This seems like material for a different patch.
Also I created an unlogged table with a SERIAL which created a
sequence. The table is unlogged but the sequence is permanent, which
is probably ok.
I only have packages down to pg 9.3, so I could not test prior 9.1.
By looking at the online documentation, is seems that relistemp
appears in pg 8.4, so the corresponding extraction should be guarded
by this version. Before that, temporary objects existed but were
identified indirectly, possibly because they were stored in a
temporary schema. I suggest not to try to address cases prior 8.4.
Done.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
v4-0001-Show-detailed-table-persistence-in-dt.patchtext/x-diff; charset=us-asciiDownload
From fd0b8215ca6bbdebc0924efaba95944731890dc9 Mon Sep 17 00:00:00 2001
From: David Fetter <david.fetter@onelogin.com>
Date: Mon, 22 Apr 2019 17:50:48 -0700
Subject: [PATCH v4] Show detailed table persistence in \dt+
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
\d would show this for individual tables, but there wasn't an
overarching view of all tables. Now, there is.
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index ee00c5da08..4aa06a417c 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3631,7 +3631,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
- static const bool translate_columns[] = {false, false, true, false, false, false, false};
+ static const bool translate_columns[] = {false, false, true, false, false, false, false, false};
/* If tabtypes is empty, we default to \dtvmsE (but see also command.c) */
if (!(showTables || showIndexes || showViews || showMatViews || showSeq || showForeign))
@@ -3680,22 +3680,36 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
if (verbose)
{
/*
- * As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
- * size of a table, including FSM, VM and TOAST tables.
+ * Show whether a table is permanent, temporary, or unlogged.
+ * Indexes are not, as of this writing, tables.
*/
- if (pset.sversion >= 90000)
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
- gettext_noop("Size"));
- else if (pset.sversion >= 80100)
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_relation_size(c.oid)) as \"%s\"",
- gettext_noop("Size"));
+ if (!showIndexes)
+ {
+ if (pset.sversion >= 91000)
+ appendPQExpBuffer(&buf,
+ ",\n CASE c.relpersistence WHEN 'p' THEN 'permanent' WHEN 't' THEN 'temporary' WHEN 'u' THEN 'unlogged' ELSE 'unknown' END as \"%s\"",
+ gettext_noop("Persistence"));
+ else if (pset.sversion >= 84000)
+ appendPQExpBuffer(&buf,
+ ",\n case when c.relistemp then 'temporary' else 'permanent' end as \"%s\"",
+ gettext_noop("Persistence"));
+ }
+ /*
+ * As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
+ * size of a table, including FSM, VM and TOAST tables.
+ */
+ if (pset.sversion >= 90000)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
+ else if (pset.sversion >= 80100)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_relation_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
- }
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_class c"
--------------2.20.1--
Hello David,
Patch applies. There seems to be a compilation issue:
describe.c:5974:1: error: expected declaration or statement at end of
input
}
Also there is an added indentation problem: the size & description stuff
have been moved left but it should still in the verbose case, and a } is
missing after them, which fixes the compilation.
Make check fails because of my temp schema was numbered 4 instead of 3, and
I'm "fabien" rather than "shackle".I think the way forward is to test this with TAP rather than the
fixed-string method.
Ok.
Checks removed while I figure out a new TAP test.
I only have packages down to pg 9.3, so I could not test prior 9.1.
By looking at the online documentation, is seems that relistemp
appears in pg 8.4, so the corresponding extraction should be guarded
by this version. Before that, temporary objects existed but were
identified indirectly, possibly because they were stored in a
temporary schema. I suggest not to try to address cases prior 8.4.Done.
After some checking, I think that there is an issue with the version
numbers:
- 9.1 is 90100, not 91000
- 8.4 is 80400, not 84000
Also, it seems that describes builds queries with uppercase keywords, so
probably the new additions should follow that style: case -> CASE (and
also when then else end as…)
--
Fabien.
On Sat, Apr 27, 2019 at 10:38:50PM +0200, Fabien COELHO wrote:
Hello David,
Patch applies. There seems to be a compilation issue:
describe.c:5974:1: error: expected declaration or statement at end of
input
}
This is in brown paper bag territory. Fixed.
I think the way forward is to test this with TAP rather than the
fixed-string method.Ok.
I've sent a separate patch extracted from the one you sent which adds
stdin to our TAP testing infrastructure. I hope it lands so it'll be
simpler to add these tests in a future version of the patch.
Checks removed while I figure out a new TAP test.
I only have packages down to pg 9.3, so I could not test prior 9.1.
By looking at the online documentation, is seems that relistemp
appears in pg 8.4, so the corresponding extraction should be guarded
by this version. Before that, temporary objects existed but were
identified indirectly, possibly because they were stored in a
temporary schema. I suggest not to try to address cases prior 8.4.Done.
After some checking, I think that there is an issue with the version
numbers:
- 9.1 is 90100, not 91000
- 8.4 is 80400, not 84000
Another brown paper bag, now fixed.
Also, it seems that describes builds queries with uppercase
keywords, so probably the new additions should follow that style:
case -> CASE (and also when then else end as…)
Done.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
v5-0001-Show-detailed-table-persistence-in-dt.patchtext/x-diff; charset=us-asciiDownload
From 3baf2d98dd699b3b04c2d3ac038dedfc9369cef7 Mon Sep 17 00:00:00 2001
From: David Fetter <david.fetter@onelogin.com>
Date: Mon, 22 Apr 2019 17:50:48 -0700
Subject: [PATCH v5] Show detailed table persistence in \dt+
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
\d would show this for individual tables, but there wasn't an
overarching view of all tables. Now, there is.
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index ee00c5da08..8b205f6e37 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3631,7 +3631,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
- static const bool translate_columns[] = {false, false, true, false, false, false, false};
+ static const bool translate_columns[] = {false, false, true, false, false, false, false, false};
/* If tabtypes is empty, we default to \dtvmsE (but see also command.c) */
if (!(showTables || showIndexes || showViews || showMatViews || showSeq || showForeign))
@@ -3680,22 +3680,37 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
if (verbose)
{
/*
- * As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
- * size of a table, including FSM, VM and TOAST tables.
+ * Show whether a table is permanent, temporary, or unlogged.
+ * Indexes are not, as of this writing, tables.
*/
- if (pset.sversion >= 90000)
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
- gettext_noop("Size"));
- else if (pset.sversion >= 80100)
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_relation_size(c.oid)) as \"%s\"",
- gettext_noop("Size"));
-
- appendPQExpBuffer(&buf,
- ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
- gettext_noop("Description"));
+ if (!showIndexes)
+ {
+ if (pset.sversion >= 90100)
+ appendPQExpBuffer(&buf,
+ ",\n CASE c.relpersistence WHEN 'p' THEN 'permanent' WHEN 't' THEN 'temporary' WHEN 'u' THEN 'unlogged' ELSE 'unknown' END as \"%s\"",
+ gettext_noop("Persistence"));
+ else if (pset.sversion >= 80400)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN c.relistemp THEN 'temporary' ELSE 'permanent' END as \"%s\"",
+ gettext_noop("Persistence"));
+ }
}
+ /*
+ * As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
+ * size of a table, including FSM, VM and TOAST tables.
+ */
+ if (pset.sversion >= 90000)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_table_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
+ else if (pset.sversion >= 80100)
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.pg_size_pretty(pg_catalog.pg_relation_size(c.oid)) as \"%s\"",
+ gettext_noop("Size"));
+
+ appendPQExpBuffer(&buf,
+ ",\n pg_catalog.obj_description(c.oid, 'pg_class') as \"%s\"",
+ gettext_noop("Description"));
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_class c"
--------------2.20.1--
Not particularly on topic, but: including a patch version number in your
subject headings is pretty unfriendly IMO, because it breaks threading
for people whose MUAs do threading by matching up subject lines.
I don't actually see the point of the [PATCH] annotation at all, because
the thread is soon going to contain lots of messages with the same subject
line but no embedded patch. Like this one. So it's just noise with no
information content worth noticing.
regards, tom lane
Hello David,
Patch applies. There seems to be a compilation issue:
describe.c:5974:1: error: expected declaration or statement at end of
input
}This is in brown paper bag territory. Fixed.
I do not understand why you move both size and description out of the
verbose mode, it should be there only when under verbose?
I've sent a separate patch extracted from the one you sent which adds
stdin to our TAP testing infrastructure. I hope it lands so it'll be
simpler to add these tests in a future version of the patch.
Why not. As I'm the one who wrote the modified function, probably I could
have thought of providing an input. I'm not sure it is worth a dedicated
submission, could go together with any commit that would use it.
--
Fabien.
On Sun, Apr 28, 2019 at 01:14:01PM -0400, Tom Lane wrote:
Not particularly on topic, but: including a patch version number in your
subject headings is pretty unfriendly IMO, because it breaks threading
for people whose MUAs do threading by matching up subject lines.
Thanks for letting me know about those MUAs.
I don't actually see the point of the [PATCH] annotation at all,
because the thread is soon going to contain lots of messages with
the same subject line but no embedded patch. Like this one. So
it's just noise with no information content worth noticing.
OK
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
On Sun, Apr 28, 2019 at 07:26:55PM +0200, Fabien COELHO wrote:
Hello David,
Patch applies. There seems to be a compilation issue:
describe.c:5974:1: error: expected declaration or statement at end of
input
}This is in brown paper bag territory. Fixed.
I do not understand why you move both size and description out of the
verbose mode, it should be there only when under verbose?
My mistake. Fixed.
I've sent a separate patch extracted from the one you sent which adds
stdin to our TAP testing infrastructure. I hope it lands so it'll be
simpler to add these tests in a future version of the patch.Why not. As I'm the one who wrote the modified function, probably I could
have thought of providing an input. I'm not sure it is worth a dedicated
submission, could go together with any commit that would use it.
My hope is that this is seen as a bug fix and gets back-patched.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
v6-0001-Show-detailed-table-persistence-in-dt.patchtext/x-diff; charset=us-asciiDownload
From ddc5dddccb275bffbb06d9bc2a5e60eced84a151 Mon Sep 17 00:00:00 2001
From: David Fetter <david.fetter@onelogin.com>
Date: Mon, 22 Apr 2019 17:50:48 -0700
Subject: [PATCH v6] Show detailed table persistence in \dt+
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
\d would show this for individual tables, but there wasn't an
overarching view of all tables. Now, there is.
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index ee00c5da08..46b1fad52d 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3631,7 +3631,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
- static const bool translate_columns[] = {false, false, true, false, false, false, false};
+ static const bool translate_columns[] = {false, false, true, false, false, false, false, false};
/* If tabtypes is empty, we default to \dtvmsE (but see also command.c) */
if (!(showTables || showIndexes || showViews || showMatViews || showSeq || showForeign))
@@ -3679,6 +3679,22 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
if (verbose)
{
+ /*
+ * Show whether a table is permanent, temporary, or unlogged.
+ * Indexes are not, as of this writing, tables.
+ */
+ if (!showIndexes)
+ {
+ if (pset.sversion >= 90100)
+ appendPQExpBuffer(&buf,
+ ",\n CASE c.relpersistence WHEN 'p' THEN 'permanent' WHEN 't' THEN 'temporary' WHEN 'u' THEN 'unlogged' ELSE 'unknown' END as \"%s\"",
+ gettext_noop("Persistence"));
+ else if (pset.sversion >= 80400)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN c.relistemp THEN 'temporary' ELSE 'permanent' END as \"%s\"",
+ gettext_noop("Persistence"));
+ }
+
/*
* As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
* size of a table, including FSM, VM and TOAST tables.
--------------2.20.1--
Hello David,
My mistake. Fixed.
About v6: applies, compiles, make check ok.
Code is ok.
Maybe there could be a comment to tell that prior version are not
addressed, something like:
...
}
/* else do not bother guessing the temporary status on old version */
No tests, pending an added TAP test infrastructure for psql.
I have a question a out the index stuff: indexes seem to appear as entries
in pg_class, and ISTM that they can be temporary/unlogged/permanent as
attached to corresponding objects. So the guard is not very useful and it
could make sense to show the information on indexes as well.
After removing the guard:
postgres=# \dtmv+ *foo*
List of relations
О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫
О©╫ Schema О©╫ Name О©╫ Type О©╫ Owner О©╫ Persistence О©╫ Size О©╫ Description О©╫
О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫
О©╫ pg_temp_3 О©╫ foo О©╫ table О©╫ fabien О©╫ temporary О©╫ 0 bytes О©╫ О©╫
О©╫ public О©╫ mfoo О©╫ materialized view О©╫ fabien О©╫ permanent О©╫ 0 bytes О©╫ О©╫
О©╫ public О©╫ ufoo О©╫ table О©╫ fabien О©╫ unlogged О©╫ 0 bytes О©╫ О©╫
О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫
(3 rows)
postgres=# \di+ *foo*
List of relations
О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫
О©╫ Schema О©╫ Name О©╫ Type О©╫ Owner О©╫ Table О©╫ Persistence О©╫ Size О©╫ Description О©╫
О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫
О©╫ pg_temp_3 О©╫ foo_pkey О©╫ index О©╫ fabien О©╫ foo О©╫ temporary О©╫ 8192 bytes О©╫ О©╫
О©╫ public О©╫ ufoo_pkey О©╫ index О©╫ fabien О©╫ ufoo О©╫ unlogged О©╫ 16 kB О©╫ О©╫
О©╫ public О©╫ ufoou О©╫ index О©╫ fabien О©╫ ufoo О©╫ unlogged О©╫ 16 kB О©╫ О©╫
О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫
(3 rows)
Is there a special reason not to show it?
--
Fabien.
On Mon, Apr 29, 2019 at 08:48:17AM +0200, Fabien COELHO wrote:
Hello David,
My mistake. Fixed.
About v6: applies, compiles, make check ok.
Code is ok.
Maybe there could be a comment to tell that prior version are not addressed,
something like:...
}
/* else do not bother guessing the temporary status on old version */
Did something like this.
No tests, pending an added TAP test infrastructure for psql.
Right.
I have a question a out the index stuff: indexes seem to appear as entries
in pg_class, and ISTM that they can be temporary/unlogged/permanent as
attached to corresponding objects. So the guard is not very useful and it
could make sense to show the information on indexes as well.
Done.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
v7-0001-Show-detailed-relation-persistence-in-dt.patchtext/x-diff; charset=us-asciiDownload
From 8fdc657cb567ec760802449da80b2aad5ae451f1 Mon Sep 17 00:00:00 2001
From: David Fetter <david.fetter@onelogin.com>
Date: Mon, 22 Apr 2019 17:50:48 -0700
Subject: [PATCH v7] Show detailed relation persistence in \dt+
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
\d would show this for individual relations, but there wasn't an
overarching view of all relations. Now, there is.
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index ee00c5da08..4c6f12ba91 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3631,7 +3631,7 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
- static const bool translate_columns[] = {false, false, true, false, false, false, false};
+ static const bool translate_columns[] = {false, false, true, false, false, false, false, false};
/* If tabtypes is empty, we default to \dtvmsE (but see also command.c) */
if (!(showTables || showIndexes || showViews || showMatViews || showSeq || showForeign))
@@ -3679,6 +3679,21 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
if (verbose)
{
+ /*
+ * Show whether a relation is permanent, temporary, or unlogged.
+ */
+ if (pset.sversion >= 90100)
+ appendPQExpBuffer(&buf,
+ ",\n CASE c.relpersistence WHEN 'p' THEN 'permanent' WHEN 't' THEN 'temporary' WHEN 'u' THEN 'unlogged' ELSE 'unknown' END as \"%s\"",
+ gettext_noop("Persistence"));
+ else if (pset.sversion >= 80400)
+ appendPQExpBuffer(&buf,
+ ",\n CASE WHEN c.relistemp THEN 'temporary' ELSE 'permanent' END as \"%s\"",
+ gettext_noop("Persistence"));
+ /*
+ * No attempt is made to show persistence in long-obsolete versions.
+ */
+
/*
* As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
* size of a table, including FSM, VM and TOAST tables.
--------------2.20.1--
Hello David,
Patch v7 applies, compiles, make check ok. No docs needed.
No tests, pending some TAP infrastructure.
I could no test with a version between 8.4 & 9.1.
No further comments. Marked as ready.
--
Fabien.
On Sat, 27 Apr 2019 at 06:18, David Fetter <david@fetter.org> wrote:
On Fri, Apr 26, 2019 at 04:22:18PM +0200, Rafia Sabih wrote:
On Fri, 26 Apr 2019 at 14:49, Rafia Sabih <rafia.pghackers@gmail.com> wrote:
On Wed, 24 Apr 2019 at 10:30, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Hello David,
I noticed that there wasn't a bulk way to see table logged-ness in psql,
so I made it part of \dt+.Applies, compiles, works for me.
ISTM That temporary-ness is not shown either. Maybe the persistence column
should be shown as is?Temporariness added, but not raw.
Ok, it is better like this way.
Tests?
Included, but they're not stable for temp tables. I'm a little stumped
as to how to either stabilize them or test some other way.Hmmm. First there is the username which appears, so there should be a
dedicated user for the test.I'm unsure how to work around the temporary schema number, which is
undeterministic with parallel execution it. I'm afraid the only viable
approach is not to show temporary tables, too bad:-(Doc?
What further documentation does it need?
Indeed, there is no precise doc, so nothing to update :-)/:-(
Maybe you could consider adding a case for prior 9.1 version, something
like:
... case c.relistemp then 'temporary' else 'permanent' end as ...I was reviewing this patch and found a bug,
create table t (i int);
create index idx on t(i);
\di+
psql: print.c:3452: printQuery: Assertion `opt->translate_columns ==
((void *)0) || opt->n_translate_columns >= cont.ncolumns' failed.Looking into this further, apparently the position of
if (verbose) { + /* + * Show whether the table is permanent, temporary, or unlogged. + */ + if (pset.sversion >= 91000) + appendPQExpBuffer(&buf, + ",\n case c.relpersistence when 'p' then 'permanent' when 't' then 'temporary' when 'u' then 'unlogged' else 'unknown' end as \"%s\"", + gettext_noop("Persistence"));is not right, it is being called for indexes with verbose option also.
There should be an extra check for it being not called for index case.
Something like,
if (verbose)
{
/*
* Show whether the table is permanent, temporary, or unlogged.
*/
if (!showIndexes)
if (pset.sversion >= 91000)
appendPQExpBuffer(&buf,
",\n case c.relpersistence when 'p' then 'permanent' when 't' then
'temporary' when 'u' then 'unlogged' else 'unknown' end as \"%s\"",
gettext_noop("Persistence"));Not sure, how do modify it in a more neat way.
I suspect that as this may get a little messier, but I've made it
fairly neat short of a major refactor.
I found the following warning on the compilation,
describe.c: In function ‘listTables’:
describe.c:3705:7: warning: this ‘if’ clause does not guard...
[-Wmisleading-indentation]
else if (pset.sversion >= 80100)
^~
describe.c:3710:3: note: ...this statement, but the latter is
misleadingly indented as if it were guarded by the ‘if’
appendPQExpBuffer(&buf,
Talking of indentation, you might want to run pgindent once. Other
than that the patch looks good to me.
--
Regards,
Rafia Sabih
David Fetter <david@fetter.org> writes:
[ v7-0001-Show-detailed-relation-persistence-in-dt.patch ]
I looked this over and had a few suggestions, as per attached v8:
* The persistence description values ought to be translatable, as
is the usual practice in describe.c. This is slightly painful
because it requires tweaking the translate_columns[] values in a
non-constant way, but it's not that bad.
* I dropped the "ELSE 'unknown'" bit in favor of just emitting NULL
if the persistence isn't recognized. This is the same way that the
table-type CASE just above does it, and I see no reason to be different.
Moreover, there are message-style-guidelines issues with what to print
if you do want to print something; "unknown" doesn't cut it.
* I also dropped the logic for pre-9.1 servers, because the existing
precedent in describeOneTableDetails() is that we only consider
relpersistence for >= 9.1, and I don't see a real good reason to
deviate from that. 9.0 and before are long out of support anyway.
If there aren't objections, I think v8 is committable.
regards, tom lane
Attachments:
v8-0001-Show-detailed-relation-persistence-in-dt.patchtext/x-diff; charset=us-ascii; name=v8-0001-Show-detailed-relation-persistence-in-dt.patchDownload
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 1c770b4..0e0af5f 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3632,7 +3632,8 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
- static const bool translate_columns[] = {false, false, true, false, false, false, false};
+ int cols_so_far;
+ bool translate_columns[] = {false, false, true, false, false, false, false, false};
/* If tabtypes is empty, we default to \dtvmsE (but see also command.c) */
if (!(showTables || showIndexes || showViews || showMatViews || showSeq || showForeign))
@@ -3672,15 +3673,40 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys
gettext_noop("partitioned index"),
gettext_noop("Type"),
gettext_noop("Owner"));
+ cols_so_far = 4;
if (showIndexes)
+ {
appendPQExpBuffer(&buf,
- ",\n c2.relname as \"%s\"",
+ ",\n c2.relname as \"%s\"",
gettext_noop("Table"));
+ cols_so_far++;
+ }
if (verbose)
{
/*
+ * Show whether a relation is permanent, temporary, or unlogged. Like
+ * describeOneTableDetails(), we consider that persistence emerged in
+ * v9.1, even though related concepts existed before.
+ */
+ if (pset.sversion >= 90100)
+ {
+ appendPQExpBuffer(&buf,
+ ",\n CASE c.relpersistence WHEN 'p' THEN '%s' WHEN 't' THEN '%s' WHEN 'u' THEN '%s' END as \"%s\"",
+ gettext_noop("permanent"),
+ gettext_noop("temporary"),
+ gettext_noop("unlogged"),
+ gettext_noop("Persistence"));
+ translate_columns[cols_so_far] = true;
+ }
+
+ /*
+ * We don't bother to count cols_so_far below here, as there's no need
+ * to; this might change with future additions to the output columns.
+ */
+
+ /*
* As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
* size of a table, including FSM, VM and TOAST tables.
*/
On 2019-Jul-02, Tom Lane wrote:
* The persistence description values ought to be translatable, as
is the usual practice in describe.c. This is slightly painful
because it requires tweaking the translate_columns[] values in a
non-constant way, but it's not that bad.
LGTM. I only fear that the cols_so_far thing is easy to break, and the
breakage will be easy to miss.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2019-Jul-02, Tom Lane wrote:
* The persistence description values ought to be translatable, as
is the usual practice in describe.c. This is slightly painful
because it requires tweaking the translate_columns[] values in a
non-constant way, but it's not that bad.
LGTM. I only fear that the cols_so_far thing is easy to break, and the
breakage will be easy to miss.
Yeah, but that's pretty true of all the translatability stuff in
describe.c. I wonder if there's any way to set up tests for that.
The fact that the .po files lag so far behind the source code seems
like an impediment --- even if we made a test case that presumed
--enable-nls and tried to exercise this, the lack of translations
for the new words would get in the way for a long while.
regards, tom lane
On 2 Jul 2019, at 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
even if we made a test case that presumed
--enable-nls and tried to exercise this, the lack of translations
for the new words would get in the way for a long while.
For testing though, couldn’t we have an autogenerated .po which has a unique
and predictable dummy value translation for every string (the string backwards
or something), which can be used for testing? This is all hand-wavy since I
haven’t tried actually doing it, but it seems a better option than waiting for
.po files to be available. Or am I missing the point of the value of the
discussed test?
cheers ./daniel
On 2019-Jul-02, Daniel Gustafsson wrote:
On 2 Jul 2019, at 22:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
even if we made a test case that presumed
--enable-nls and tried to exercise this, the lack of translations
for the new words would get in the way for a long while.For testing though, couldn’t we have an autogenerated .po which has a unique
and predictable dummy value translation for every string (the string backwards
or something), which can be used for testing? This is all hand-wavy since I
haven’t tried actually doing it, but it seems a better option than waiting for
.po files to be available. Or am I missing the point of the value of the
discussed test?
Hmm, no, I think that's precisely it, and that sounds like a pretty good
starter idea ... but I wouldn't want to be the one to have to set this
up -- it seems pretty laborious.
Anyway I'm not objecting to the patch -- I agree that we're already not
testing translatability and that this patch shouldn't be forced to start
doing it.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2 Jul 2019, at 22:35, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Anyway I'm not objecting to the patch -- I agree that we're already not
testing translatability and that this patch shouldn't be forced to start
doing it.
I forgot to add that to my previous email, the patch as it stands in v8 looks
good to me. I’ve missed having this on many occasions.
cheers ./daniel
Daniel Gustafsson <daniel@yesql.se> writes:
On 2 Jul 2019, at 22:35, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Anyway I'm not objecting to the patch -- I agree that we're already not
testing translatability and that this patch shouldn't be forced to start
doing it.
I forgot to add that to my previous email, the patch as it stands in v8 looks
good to me. I’ve missed having this on many occasions.
OK, pushed.
For the record, I did verify that the translatability logic worked
by adding some bogus entries to psql/po/es.po and seeing that the
display changed to match.
regards, tom lane