Re: [PATCH v5] Show detailed table persistence in \dt+
Daniel Gustafsson writes: >> On 2 Jul 2019, at 22:35, Alvaro Herrera 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
Re: [PATCH v5] Show detailed table persistence in \dt+
> On 2 Jul 2019, at 22:35, Alvaro Herrera 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
Re: [PATCH v5] Show detailed table persistence in \dt+
On 2019-Jul-02, Daniel Gustafsson wrote: > > On 2 Jul 2019, at 22:16, Tom Lane 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH v5] Show detailed table persistence in \dt+
> On 2 Jul 2019, at 22:16, Tom Lane 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
Re: [PATCH v5] Show detailed table persistence in \dt+
Alvaro Herrera 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
Re: [PATCH v5] Show detailed table persistence in \dt+
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH v5] Show detailed table persistence in \dt+
David Fetter 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
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.
*/
Re: [PATCH v5] Show detailed table persistence in \dt+
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.
Re: [PATCH v5] Show detailed table persistence in \dt+
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 http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From 8fdc657cb567ec760802449da80b2aad5ae451f1 Mon Sep 17 00:00:00 2001
From: David Fetter
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--
Re: [PATCH v5] Show detailed table persistence in \dt+
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.
Re: [PATCH v5] Show detailed table persistence in \dt+
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 http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From ddc5dddccb275bffbb06d9bc2a5e60eced84a151 Mon Sep 17 00:00:00 2001
From: David Fetter
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--
Re: [PATCH v5] Show detailed table persistence in \dt+
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 http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: [PATCH v5] Show detailed table persistence in \dt+
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.
Re: [PATCH v5] Show detailed table persistence in \dt+
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
