From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Fetter <david@fetter.org>
cc: Fabien COELHO <coelho@cri.ensmp.fr>,
        Rafia Sabih <rafia.pghackers@gmail.com>,
        PostgreSQL Development <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH v5] Show detailed table persistence in \dt+
In-reply-to: <20190429142358.GE2954@fetter.org>
References: 
 <CA+FpmFdx2aDtyiVD2av9+QAunA1L5uscm3dL1SFbcOs8-W678g@mail.gmail.com>
 <CA+FpmFeurtDiutfh2yv0T4S=zY0iegcSStR-5xJ9H7vP2XKQsg@mail.gmail.com>
 <20190427041849.GL28936@fetter.org>
 <alpine.DEB.2.21.1904270844180.29865@lancre>
 <20190427174330.GN28936@fetter.org>
 <alpine.DEB.2.21.1904272214030.29865@lancre>
 <20190428151541.GQ28936@fetter.org>
 <alpine.DEB.2.21.1904281917330.29865@lancre>
 <20190429041902.GC2954@fetter.org>
 <alpine.DEB.2.21.1904290831550.8815@lancre>
 <20190429142358.GE2954@fetter.org>
Comments: In-reply-to David Fetter <david@fetter.org>
	message dated "Mon, 29 Apr 2019 16:23:58 +0200"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <4497.1562094570.0@sss.pgh.pa.us>
Date: Tue, 02 Jul 2019 15:10:37 -0400
Message-ID: <4561.1562094637@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4497.1562094570.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="v8-0001-Show-detailed-relation-persistence-in-dt.patch";
	charset="us-ascii"
Content-ID: <4497.1562094570.2@sss.pgh.pa.us>
Content-Description: v8-0001-Show-detailed-relation-persistence-in-dt.patch
Content-Transfer-Encoding: quoted-printable

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 =3D pset.popt;
-	static const bool translate_columns[] =3D {false, false, true, false, fa=
lse, false, false};
+	int			cols_so_far;
+	bool		translate_columns[] =3D {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 *patte=
rn, bool verbose, bool showSys
 					  gettext_noop("partitioned index"),
 					  gettext_noop("Type"),
 					  gettext_noop("Owner"));
+	cols_so_far =3D 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 >=3D 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] =3D 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.
 		 */

------- =_aaaaaaaaaa0--