Thread: v13: show extended stats target in \d
The stats target can be set since commit d06215d03, but wasn't shown by psql. ALTER STATISISTICS .. SET STATISTICS n. Normal (1-D) stats targets are shown in \d+ table. Stats objects are shown in \d (no plus). Arguably, this should be shown only in "verbose" mode (\d+).
Attachment
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Monday, 31 August 2020 08:00, Justin Pryzby <pryzby@telsasoft.com> wrote: > The stats target can be set since commit d06215d03, but wasn't shown by psql. > ALTER STATISISTICS .. SET STATISTICS n. > > Normal (1-D) stats targets are shown in \d+ table. > Stats objects are shown in \d (no plus). > Arguably, this should be shown only in "verbose" mode (\d+). This seems rather useful. May I suggest you add it to the commitfest? //Georgios
On Mon, Aug 31, 2020 at 07:47:35AM +0000, gkokolatos@pm.me wrote: > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Monday, 31 August 2020 08:00, Justin Pryzby <pryzby@telsasoft.com> wrote: > > > The stats target can be set since commit d06215d03, but wasn't shown by psql. > > ALTER STATISISTICS .. SET STATISTICS n. > > > > Normal (1-D) stats targets are shown in \d+ table. > > Stats objects are shown in \d (no plus). > > Arguably, this should be shown only in "verbose" mode (\d+). > > This seems rather useful. May I suggest you add it to the commitfest? I added at https://commitfest.postgresql.org/29/2712/ + appendPQExpBuffer(&buf, " STATISTICS %s", Maybe it should have a comma, like ", STATISTICS %s"? Similar to these: appendPQExpBuffer(&buf, ", ON TABLE %s", |Triggers: | trg1 AFTER INSERT ON trigpart3 FOR EACH ROW EXECUTE FUNCTION trigger_nothing(), ON TABLE trigpart and: appendPQExpBufferStr(&buf, ", PARTITIONED"); |part_ee_ff FOR VALUES IN ('ee', 'ff'), PARTITIONED, Also, now I wonder if CREATE STATISTICS should support some syntax to set the initial target. Like: |CREATE STATISTICS abstats ON a,b FROM child.abc_202006 WITH(STATISTICS 111); -- Justin
Hi Justin, On 2020/08/31 14:00, Justin Pryzby wrote: > The stats target can be set since commit d06215d03, but wasn't shown by psql. > ALTER STATISISTICS .. SET STATISTICS n. > > Normal (1-D) stats targets are shown in \d+ table. > Stats objects are shown in \d (no plus). > Arguably, this should be shown only in "verbose" mode (\d+). I tested your patch on 13beta3 and head (ab3c6d41). It looks good to me. :-D Thanks, Tatsuro Yamada
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
Hi,
I will humbly disagree with the current review. I shall refrain from changing the status though because I do not have
verystrong feeling about it.
I am in agreement that it is a helpful feature and as implemented, the result seems to be the desired one.
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
andclean. So in my humble opinion, the ternary expression should be replaced with a distinct if block that would
implementstxstattarget. Second, setting the value to -1 as an indication of absence breaks the pattern a bit further.
Moreon that bellow.
+ 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));
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
youdiscard it?
Regards,
Georgios
+1 on fixing this, since the ability to change stats target is new in
pg13.
On 2020-Aug-31, Justin Pryzby wrote:
> Maybe it should have a comma, like ", STATISTICS %s"?
It does need some separator. Maybe a comma is sufficient, but I'm not
sure: that will fail when we add cross-relation stats, because the
FROM clause will have more relations and possibly have commas too.
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
> Also, now I wonder if CREATE STATISTICS should support some syntax to set the
> initial target. Like:
>
> |CREATE STATISTICS abstats ON a,b FROM child.abc_202006 WITH(STATISTICS 111);
Umm, if true (on which I'm not sold), maybe it should appear in the
parenthesized list that currently is just the stats type list:
|CREATE STATISTICS abstats (STATISTICS 111) ON a,b FROM child.abc_202006;
I'm not really convinced we need this.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Sep 01, 2020 at 05:08:25PM -0400, Alvaro Herrera wrote: > +1 on fixing this, since the ability to change stats target is new in > pg13. > > On 2020-Aug-31, Justin Pryzby wrote: > > > Maybe it should have a comma, like ", STATISTICS %s"? > > It does need some separator. Maybe a comma is sufficient, but I'm not > sure: that will fail when we add cross-relation stats, because the > FROM clause will have more relations and possibly have commas too. Good thought. You said it'll "fail", but I guess you mean it could be confusing to look at. I didn't actually know that "multiple tables" were planned for MV stats. Another consideration is expression stats (as compared with expression indexes). I don't see how that helps guide this decision at all, though. I think trying to parse \d output is discouraged and a bad idea, but it's obviously not ok if the output is ambiguous. But it's not ambiguous, since STATISTICS is capitalized and unquoted. Arguably, "nn" could be construed as looking like a table alias, which doesn't make sense here, and integer aliases would also need to be quoted (and seem unlikely and not useful). ... FROM t1, t2, STATISTICS nn I think this is 1) unambiguous; 2) clear; 3) consistent with other output and with the "ALTER STATISTICS SET STATISTICS NN" command. It could say "SET" but I don't think it's useful to include; 4) seems to reasonably anticipate future support for expressions and multiple tables; I'm happy if someone suggests something better, but I don't know what that would be. Unless we hurried up and finished this for v13, and included stxstattarget. https://commitfest.postgresql.org/29/2692/ https://www.postgresql.org/message-id/flat/c0939aba-3b12-b596-dd08-913dda4b40f0%40nttcom.co.jp_1#32680b2fe0ab473c58a33eb0f9f00d42 Maybe it's not relevant, but I found these don't have a separator. ppendPQExpBufferStr(&buf, " DEFERRABLE"); ppendPQExpBufferStr(&buf, " INITIALLY DEFERRED"); ppendPQExpBufferStr(&buf, " INVALID"); ppendPQExpBufferStr(&buf, " PRIMARY KEY,"); ppendPQExpBufferStr(&buf, " REPLICA IDENTITY"); ppendPQExpBufferStr(&buf, " UNIQUE,"); ppendPQExpBufferStr(&buf, " UNIQUE CONSTRAINT,"); ppendPQExpBufferStr(&buf, " CLUSTER"); ppendPQExpBufferStr(&buf, " AS RESTRICTIVE"); ppendPQExpBuffer(&buf, "\n TO %s", These look like the only similar things with more than a single separator: ppendPQExpBuffer(&buf, "\n USING (%s)", ppendPQExpBuffer(&buf, "\n WITH CHECK (%s)", -- Justin
On Tue, Sep 1, 2020 at 2:08 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > It does need some separator. Maybe a comma is sufficient, but I'm not > sure: that will fail when we add cross-relation stats, because the > FROM clause will have more relations and possibly have commas too. How about a line break? That seems like a simple solution that takes all the competing concerns into account. The fact that that will stand out isn't necessarily a bad thing. I think it's a good thing. -- Peter Geoghegan
On Sun, Sep 06, 2020 at 01:06:05PM -0700, Peter Geoghegan wrote:
> On Tue, Sep 1, 2020 at 2:08 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > It does need some separator. Maybe a comma is sufficient, but I'm not
> > sure: that will fail when we add cross-relation stats, because the
> > FROM clause will have more relations and possibly have commas too.
>
> How about a line break? That seems like a simple solution that takes
> all the competing concerns into account.
>
> The fact that that will stand out isn't necessarily a bad thing. I
> think it's a good thing.
Like this ?
postgres=# \d t
Table "public.t"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | |
b | integer | | |
Statistics objects:
"public"."t" (ndistinct, dependencies, mcv) ON a, b FROM t
Are there any other examples of similarly related information spread across
lines ?
I find that to be too verbose ; I guess it could be shown only in \d+, which is
true of column stats.
It's weird that the quoting rules are different for the stats object vs the
columns and the table. The schema qualification is also divergent. Also,
"public.t" form is technically wrong (the objects should be *separately*
quoted), but seems to be used all over. If the table or schema has a dot, it's
ambiguous what this means: Table "public.public.t".
--
Justin
On Sun, Sep 6, 2020 at 1:48 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Sun, Sep 06, 2020 at 01:06:05PM -0700, Peter Geoghegan wrote:
> > How about a line break? That seems like a simple solution that takes
> > all the competing concerns into account.
> Like this ?
> Statistics objects:
> "public"."t" (ndistinct, dependencies, mcv) ON a, b FROM t
No, not like that.
ISTM that the problem with your original proposal is that it suggests
that it ought to be possible to add "STATISTICS 0" to the end of a
CREATE STATISTICS statement. That's not accurate, though. In reality
the only way to set the statistics target for a statistics object is
with ALTER STATISTICS. We should attempt to convey that difference
here.
More concretely, I was suggesting including a second line that's
similar to the following example output (iff the statistics target has
actually been set):
Statistics objects:
"public"."t" (ndistinct, dependencies, mcv) ON a, b FROM t
(STATISTICS 0)
Maybe some variation would be better -- the precise details are not
important. The new line conveys the fact that STATISTICS 0 is a part
of the object, but cannot appear in CREATE STATISTICS itself. That
seems like the right way of framing this.
--
Peter Geoghegan
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 verystrong 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 helpfuland clean. So in my humble opinion, the ternary expression should be replaced with a distinct if block that wouldimplement 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 youdiscard 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
Attachment
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 10 September 2020 00:36, Justin Pryzby <pryzby@telsasoft.com> wrote: > 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 havevery strong feeling about it. > > Sorry but this was in my spam, and didn't see until now. No worries at all. Thank you for replying. > > > 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 helpfuland clean. So in my humble opinion, the ternary expression should be replaced with a distinct if block that wouldimplement 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. Thank you for taking my input. > > > - 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. Fair enough. As I said above, I do not have a very strong feeling, so it gets my +1 if it is worth anything. > > > 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 didyou 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 Yeah, I have noticed that. Hence my question. If anything maybe the documentation can be expanded to cover these cases ina different patch. Thank you for your answer. > > 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 >