Re: v13: show extended stats target in \d - Mailing list pgsql-hackers

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