Re: v13: show extended stats target in \d
От | Georgios Kokolatos |
---|---|
Тема | Re: v13: show extended stats target in \d |
Дата | |
Msg-id | 159896371909.18329.12822998639760786889.pgcf@coridan.postgresql.org обсуждение исходный текст |
Ответ на | Re: v13: show extended stats target in \d (Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>) |
Ответы |
Re: v13: show extended stats target in \d
(Justin Pryzby <pryzby@telsasoft.com>)
|
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Surafel TemesgenДата:
Сообщение: Re: Evaluate expression at planning time for two more cases