Date: Wed, 9 Sep 2020 16:36:27 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Georgios Kokolatos <gkokolatos@protonmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: v13: show extended stats target in \d
Message-ID: <20200909213627.GW18552@telsasoft.com>
References: <20200831050047.GG5450@telsasoft.com>
 <61159ce4-069e-1b12-5415-31d09934dd5f@nttcom.co.jp_1>
 <159896371909.18329.12822998639760786889.pgcf@coridan.postgresql.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="s9fJI615cBHmzTOP"
Content-Disposition: inline
In-Reply-To: 
 <159896371909.18329.12822998639760786889.pgcf@coridan.postgresql.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--s9fJI615cBHmzTOP
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Sep 01, 2020 at 12:35:19PM +0000, Georgios Kokolatos wrote:
> I will humbly disagree with the current review. I shall refrain from changing the status though because I do not have very strong feeling about it.

Sorry but this was in my spam, and didn't see until now.

> 
> However the patch contains:
> 
> -                                                         "  'm' = any(stxkind) AS mcv_enabled\n"
> +                                                         "  'm' = any(stxkind) AS mcv_enabled,\n"
> +                                                         "  %s"
>                                                           "FROM pg_catalog.pg_statistic_ext stat "
>                                                           "WHERE stxrelid = '%s'\n"
>                                                           "ORDER BY 1;",
> +                                                         (pset.sversion >= 130000 ? "stxstattarget\n" : "-1\n"),
>                                                           oid);
> 
> This seems to be breaking a bit the pattern in describeOneTableDetails().
> First, it is customary to write the whole query for the version in its own block. I do find this pattern rather helpful and clean. So in my humble opinion, the ternary expression should be replaced with a distinct if block that would implement stxstattarget. Second, setting the value to -1 as an indication of absence breaks the pattern a bit further. More on that bellow.

Hm, I did like this using the "hastriggers" code as a template.  But you're
right that everywhere else does it differently.

> +                                       if (strcmp(PQgetvalue(result, i, 8), "-1") != 0)
> +                                               appendPQExpBuffer(&buf, " STATISTICS %s",
> +                                                                         PQgetvalue(result, i, 8));
> +
> 
> In the same function, one will find a bit of a different pattern for printing the statistics, e.g.
>      if (strcmp(PQgetvalue(result, i, 7), "t") == 0) 
> I will again hold the opinion that if the query gets crafted a bit differently, one can inspect if the table has `stxstattarget` and then, go ahead and print the value.
> 
> Something in the lines of:
>                     if (strcmp(PQgetvalue(result, i, 8), "t") == 0)
>                         appendPQExpBuffer(&buf, " STATISTICS %s", PQgetvalue(result, i, 9));

I think what you've written wouldn't give the desired behavior, since it would
show the stats target even when it's set to the default.  I don't see the point
of having additional, separate, version-specific boolean columns for 1) column
exists; 2) column is not default, in addition to 3) column value.  But I added
comment about what Peter and I agree is desirable, anyway.

> Finally, the patch might be more complete if a note is added in the documentation.
> Have you considered adding something in doc/src/sgml/ref/psql-ref.sgml? If no, will you consider it? If yes, why did you discard it?

I don't think the details of psql output are currently documented.  This shows
nothing about column statistics target, nor about MV stats at all.
https://www.postgresql.org/docs/13/app-psql.html

As for the discussion about a separator, I think maybe a comma is enough.  I
doubt anyone is going to think that you can get a valid command by prefixing
this by "CREATE STATISTICS".  Actually, it didn't even occur to me it was valid
command without the stats target - after all, that's not true of indexes.

+    "public"."ab1_a_b_stats" (ndistinct, dependencies, mcv) ON a, b FROM ab1, STATISTICS 0

This revision only shows the stats target in verbose mode (slash dee plus).

-- 
Justin

--s9fJI615cBHmzTOP
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v2-0001-Show-stats-target-of-extended-statistics.patch"

From e5b351cc9b0518c9162a4b988b17ed920f09d952 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 30 Aug 2020 23:38:17 -0500
Subject: [PATCH v2] Show stats target of extended statistics

The stats target can be set since commit d06215d03, but wasn't shown by psql.
 ALTER STATISISTICS .. SET STATISTICS n.
Traditional column stats targets are shown in \d+ table, so do the same for
extended/MV stats.
---
 src/bin/psql/describe.c                 | 14 ++++++++++++--
 src/test/regress/expected/stats_ext.out | 18 ++++++++++++++++++
 src/test/regress/sql/stats_ext.sql      |  2 ++
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 0861d74a6f..3c391fe686 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2683,8 +2683,13 @@ describeOneTableDetails(const char *schemaname,
 							  "        a.attnum = s.attnum AND NOT attisdropped)) AS columns,\n"
 							  "  'd' = any(stxkind) AS ndist_enabled,\n"
 							  "  'f' = any(stxkind) AS deps_enabled,\n"
-							  "  'm' = any(stxkind) AS mcv_enabled\n"
-							  "FROM pg_catalog.pg_statistic_ext stat "
+							  "  'm' = any(stxkind) AS mcv_enabled");
+
+			if (pset.sversion >= 130000)
+				appendPQExpBufferStr(&buf, ",\n  stxstattarget\n");
+			else
+				appendPQExpBufferStr(&buf, ",\n  -1 AS stxstattarget\n");
+			appendPQExpBuffer(&buf, "FROM pg_catalog.pg_statistic_ext stat "
 							  "WHERE stxrelid = '%s'\n"
 							  "ORDER BY 1;",
 							  oid);
@@ -2732,6 +2737,11 @@ describeOneTableDetails(const char *schemaname,
 									  PQgetvalue(result, i, 4),
 									  PQgetvalue(result, i, 1));
 
+					/* Show the stats target if it's not default */
+					if (verbose && strcmp(PQgetvalue(result, i, 8), "-1") != 0)
+						appendPQExpBuffer(&buf, ", STATISTICS %s",
+									  PQgetvalue(result, i, 8));
+
 					printTableAddFooter(&cont, buf.data);
 				}
 			}
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index 8c667d786a..aeea0e46a7 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -102,6 +102,15 @@ WARNING:  statistics object "public.ab1_a_b_stats" could not be computed for rel
 ALTER TABLE ab1 ALTER a SET STATISTICS -1;
 -- setting statistics target 0 skips the statistics, without printing any message, so check catalog
 ALTER STATISTICS ab1_a_b_stats SET STATISTICS 0;
+\d+ ab1
+                                    Table "public.ab1"
+ Column |  Type   | Collation | Nullable | Default | Storage | Stats target | Description 
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ a      | integer |           |          |         | plain   |              | 
+ b      | integer |           |          |         | plain   |              | 
+Statistics objects:
+    "public"."ab1_a_b_stats" (ndistinct, dependencies, mcv) ON a, b FROM ab1, STATISTICS 0
+
 ANALYZE ab1;
 SELECT stxname, stxdndistinct, stxddependencies, stxdmcv
   FROM pg_statistic_ext s, pg_statistic_ext_data d
@@ -113,6 +122,15 @@ SELECT stxname, stxdndistinct, stxddependencies, stxdmcv
 (1 row)
 
 ALTER STATISTICS ab1_a_b_stats SET STATISTICS -1;
+\d+ ab1
+                                    Table "public.ab1"
+ Column |  Type   | Collation | Nullable | Default | Storage | Stats target | Description 
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ a      | integer |           |          |         | plain   |              | 
+ b      | integer |           |          |         | plain   |              | 
+Statistics objects:
+    "public"."ab1_a_b_stats" (ndistinct, dependencies, mcv) ON a, b FROM ab1
+
 -- partial analyze doesn't build stats either
 ANALYZE ab1 (a);
 WARNING:  statistics object "public.ab1_a_b_stats" could not be computed for relation "public.ab1"
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index f8d947af9e..ee522d83b1 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -72,12 +72,14 @@ ANALYZE ab1;
 ALTER TABLE ab1 ALTER a SET STATISTICS -1;
 -- setting statistics target 0 skips the statistics, without printing any message, so check catalog
 ALTER STATISTICS ab1_a_b_stats SET STATISTICS 0;
+\d+ ab1
 ANALYZE ab1;
 SELECT stxname, stxdndistinct, stxddependencies, stxdmcv
   FROM pg_statistic_ext s, pg_statistic_ext_data d
  WHERE s.stxname = 'ab1_a_b_stats'
    AND d.stxoid = s.oid;
 ALTER STATISTICS ab1_a_b_stats SET STATISTICS -1;
+\d+ ab1
 -- partial analyze doesn't build stats either
 ANALYZE ab1 (a);
 ANALYZE ab1;
-- 
2.17.0


--s9fJI615cBHmzTOP--