pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedRe: v13: show extended stats target in \d
9+ messages / 6 participants
[nested] [flat]
* Re: v13: show extended stats target in \d
@ 2020-09-01 01:14 Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 1 reply; 9+ messages in thread
From: Justin Pryzby @ 2020-09-01 01:14 UTC (permalink / raw)
To: gkokolatos@pm.me; +Cc: pgsql-hackers@postgresql.org, Tomas Vondra <tomas.vondra@2ndquadrant.com>; Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: v13: show extended stats target in \d
@ 2020-09-01 01:37 Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>
1 sibling, 1 reply; 9+ messages in thread
From: Tatsuro Yamada @ 2020-09-01 01:37 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers; +Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: v13: show extended stats target in \d
@ 2020-09-01 12:35 Georgios Kokolatos <gkokolatos@protonmail.com>
parent: Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>
0 siblings, 0 replies; 9+ messages in thread
From: Georgios Kokolatos @ 2020-09-01 12:35 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Justin Pryzby <justin@telsasoft.com>
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 very strong 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 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.
+ 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 you discard it?
Regards,
Georgios
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: v13: show extended stats target in \d
@ 2020-09-01 21:08 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 3 replies; 9+ messages in thread
From: Alvaro Herrera @ 2020-09-01 21:08 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: gkokolatos@pm.me, pgsql-hackers@postgresql.org, Tomas Vondra <tomas.vondra@2ndquadrant.com>; Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>
+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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: v13: show extended stats target in \d
@ 2020-09-01 21:22 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
2 siblings, 0 replies; 9+ messages in thread
From: Tomas Vondra @ 2020-09-01 21:22 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; gkokolatos@pm.me, pgsql-hackers@postgresql.org, Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>
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.
>
> 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.
>
Me neither. IMHO it's somewhat similar to per-column statistics target,
where we don't allow specifying that in CREATE TABLE and you have to do
ALTER TABLE ... ALTER COLUMN ... SET STATISTICS n;
as a separate step.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: v13: show extended stats target in \d
@ 2020-09-05 21:06 Justin Pryzby <pryzby@telsasoft.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
2 siblings, 0 replies; 9+ messages in thread
From: Justin Pryzby @ 2020-09-05 21:06 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: gkokolatos@pm.me, pgsql-hackers@postgresql.org, Tomas Vondra <tomas.vondra@2ndquadrant.com>; Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>
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#326...
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: v13: show extended stats target in \d
@ 2020-09-06 20:06 Peter Geoghegan <pg@bowt.ie>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
2 siblings, 1 reply; 9+ messages in thread
From: Peter Geoghegan @ 2020-09-06 20:06 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Georgios Kokolatos <gkokolatos@pm.me>; pgsql-hackers; Tomas Vondra <tomas.vondra@2ndquadrant.com>; Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: v13: show extended stats target in \d
@ 2020-09-06 20:48 Justin Pryzby <pryzby@telsasoft.com>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 9+ messages in thread
From: Justin Pryzby @ 2020-09-06 20:48 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Georgios Kokolatos <gkokolatos@pm.me>; pgsql-hackers; Tomas Vondra <tomas.vondra@2ndquadrant.com>; Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: v13: show extended stats target in \d
@ 2020-09-07 21:31 Peter Geoghegan <pg@bowt.ie>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 0 replies; 9+ messages in thread
From: Peter Geoghegan @ 2020-09-07 21:31 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Georgios Kokolatos <gkokolatos@pm.me>; pgsql-hackers; Tomas Vondra <tomas.vondra@2ndquadrant.com>; Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>
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
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2020-09-07 21:31 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-01 01:14 ` Justin Pryzby <pryzby@telsasoft.com>
2020-09-01 21:08 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-01 21:22 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2020-09-05 21:06 ` Justin Pryzby <pryzby@telsasoft.com>
2020-09-06 20:06 ` Peter Geoghegan <pg@bowt.ie>
2020-09-06 20:48 ` Justin Pryzby <pryzby@telsasoft.com>
2020-09-07 21:31 ` Peter Geoghegan <pg@bowt.ie>
2020-09-01 01:37 ` Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>
2020-09-01 12:35 ` Georgios Kokolatos <gkokolatos@protonmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox