‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, 10 September 2020 00:36, Justin Pryzby <pry...@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 have very 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 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.


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 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

Yeah, I have noticed that. Hence my question. If anything maybe the 
documentation can be expanded to cover these cases in a 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
>




Reply via email to