From: Georgios Kokolatos <gkokolatos@protonmail.com>
To: pgsql-hackers@lists.postgresql.org
Cc: Justin Pryzby <justin@telsasoft.com>
Subject: Re: v13: show extended stats target in \d
Date: Tue, 01 Sep 2020 12:35:19 +0000
Message-ID: <159896371909.18329.12822998639760786889.pgcf@coridan.postgresql.org> (raw)
In-Reply-To: <61159ce4-069e-1b12-5415-31d09934dd5f@nttcom.co.jp_1>
References: <20200831050047.GG5450@telsasoft.com>
<61159ce4-069e-1b12-5415-31d09934dd5f@nttcom.co.jp_1>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: gkokolatos@protonmail.com, pgsql-hackers@lists.postgresql.org, justin@telsasoft.com
Subject: Re: v13: show extended stats target in \d
In-Reply-To: <159896371909.18329.12822998639760786889.pgcf@coridan.postgresql.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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