Add prefix pg_catalog to pg_get_statisticsobjdef_columns() in describe.c (\dX)
Hi,
I found other functions that we should add "pg_catalog" prefix in
describe.c. This fix is similar to the following commit.
=====
commit 359bcf775550aa577c86ea30a6d071487fcca1ed
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Sat Aug 28 12:04:15 2021 -0400
psql \dX: reference regclass with "pg_catalog." prefix
=====
Please find attached the patch.
Regards,
Tatsuro Yamada
Attachments:
add_prefix_pg_catalog.patchtext/plain; charset=UTF-8; name=add_prefix_pg_catalog.patchDownload
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 90ff649..a33d77c 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2881,7 +2881,7 @@ describeOneTableDetails(const char *schemaname,
"stxrelid::pg_catalog.regclass, "
"stxnamespace::pg_catalog.regnamespace AS nsp, "
"stxname,\n"
- "pg_get_statisticsobjdef_columns(oid) AS columns,\n"
+ "pg_catalog.pg_get_statisticsobjdef_columns(oid) AS columns,\n"
" 'd' = any(stxkind) AS ndist_enabled,\n"
" 'f' = any(stxkind) AS deps_enabled,\n"
" 'm' = any(stxkind) AS mcv_enabled,\n"
@@ -4734,7 +4734,7 @@ listExtendedStats(const char *pattern)
if (pset.sversion >= 140000)
appendPQExpBuffer(&buf,
"pg_catalog.format('%%s FROM %%s', \n"
- " pg_get_statisticsobjdef_columns(es.oid), \n"
+ " pg_catalog.pg_get_statisticsobjdef_columns(es.oid), \n"
" es.stxrelid::pg_catalog.regclass) AS \"%s\"",
gettext_noop("Definition"));
else
On Mon, Sep 27, 2021 at 2:14 AM Tatsuro Yamada <
tatsuro.yamada.tf@nttcom.co.jp> wrote:
Hi,
I found other functions that we should add "pg_catalog" prefix in
describe.c. This fix is similar to the following commit.
Hi!
Yup, that's correct. Applied and backpatched to 14 (but it won't be in the
14.0 release).
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
Hi Magnus!
I found other functions that we should add "pg_catalog" prefix in
describe.c. This fix is similar to the following commit.Yup, that's correct. Applied and backpatched to 14 (but it won't be in the 14.0 release).
Thank you!
Regards,
Tatsuro Yamada
/messages/by-id/7ad8cd13-db5b-5cf6-8561-dccad1a934cb@nttcom.co.jp
/messages/by-id/20210827193151.GN26465@telsasoft.com
On Sat, Aug 28, 2021 at 08:57:32AM -0400, �lvaro Herrera wrote:
On 2021-Aug-27, Justin Pryzby wrote:
I noticed that for \dP+ since 1c5d9270e, regclass is written without
"pg_catalog." (Alvaro and I failed to notice it in 421a2c483, too).Oops, will fix shortly.
On Tue, Sep 28, 2021 at 04:25:11PM +0200, Magnus Hagander wrote:
On Mon, Sep 27, 2021 at 2:14 AM Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp> wrote:
I found other functions that we should add "pg_catalog" prefix in
describe.c. This fix is similar to the following commit.Yup, that's correct. Applied and backpatched to 14 (but it won't be in the
14.0 release).
Those two issues were fixed in 1f092a309 and 07f8a9e784.
But, we missed two casts to ::text which don't use pg_catalog.
Evidently the cast is to allow stable sorting.
I improved on my previous hueristic to look for these ; this finds the two
missing schema qualifiers:
time grep "$(sed -r "/.*pg_catalog\\.([_[:alpha:]]+).*/! d; s//\\1/; /^(char|oid)$/d; s/.*/[^. ']\\\\<&\\\\>/" src/bin/psql/describe.c |sort -u)" src/bin/psql/describe.c
While looking at that, I wondered why describeOneTableDetails lists stats
objects in order of OID ? Dating back to 7b504eb28, and, before that,
554A73A6.2060603@2ndquadrant.com. It should probably order by nsp, stxname.
Hi Justin,
On 2022/01/07 11:22, Justin Pryzby wrote:
But, we missed two casts to ::text which don't use pg_catalog.
Evidently the cast is to allow stable sorting.
Ah, you are right.
We should prefix them with pg_catalog as well.
Are you planning to make a patch?
If not, I'll make a patch later since that's where \dX is.
Regards,
Tatsuro Yamada
On Fri, Jan 07, 2022 at 06:30:30PM +0900, Tatsuro Yamada wrote:
We should prefix them with pg_catalog as well.
Are you planning to make a patch?
If not, I'll make a patch later since that's where \dX is.
If any of you can make a patch, that would be great. Thanks!
--
Michael
On Fri, Jan 07, 2022 at 08:08:57PM +0900, Michael Paquier wrote:
On Fri, Jan 07, 2022 at 06:30:30PM +0900, Tatsuro Yamada wrote:
We should prefix them with pg_catalog as well.
Are you planning to make a patch?
If not, I'll make a patch later since that's where \dX is.If any of you can make a patch, that would be great. Thanks!
I'd propose these.
Attachments:
0001-psql-qualify-text-with-pg_catalog.patchtext/x-diff; charset=us-asciiDownload
From d1e337daf81faa9a0a8c33a26091cbae396df7bb Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 6 Jan 2022 20:23:52 -0600
Subject: [PATCH 1/3] psql: qualify ::text with pg_catalog
backpatch to v14
---
src/bin/psql/describe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 7cbf5f7b105..b3fbff1a0c3 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -4392,7 +4392,7 @@ listExtendedStats(const char *pattern)
initPQExpBuffer(&buf);
printfPQExpBuffer(&buf,
"SELECT \n"
- "es.stxnamespace::pg_catalog.regnamespace::text AS \"%s\", \n"
+ "es.stxnamespace::pg_catalog.regnamespace::pg_catalog.text AS \"%s\", \n"
"es.stxname AS \"%s\", \n",
gettext_noop("Schema"),
gettext_noop("Name"));
@@ -4439,7 +4439,7 @@ listExtendedStats(const char *pattern)
processSQLNamePattern(pset.db, &buf, pattern,
false, false,
- "es.stxnamespace::pg_catalog.regnamespace::text", "es.stxname",
+ "es.stxnamespace::pg_catalog.regnamespace::pg_catalog.text", "es.stxname",
NULL, "pg_catalog.pg_statistics_obj_is_visible(es.oid)");
appendPQExpBufferStr(&buf, "ORDER BY 1, 2;");
--
2.17.1
0002-psql-order-a-relation-s-extended-statistics-by-name.patchtext/x-diff; charset=us-asciiDownload
From 5033095abb3631bf01e4213752d5a4d3a24896fa Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 6 Jan 2022 20:25:07 -0600
Subject: [PATCH 2/3] psql: order a relation's extended statistics by name..
like \dX
not by OID
v15 only?
---
src/bin/psql/describe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index b3fbff1a0c3..daf06bbcc81 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2608,7 +2608,7 @@ describeOneTableDetails(const char *schemaname,
printfPQExpBuffer(&buf,
"SELECT oid, "
"stxrelid::pg_catalog.regclass, "
- "stxnamespace::pg_catalog.regnamespace AS nsp, "
+ "stxnamespace::pg_catalog.regnamespace::pg_catalog.text AS nsp, "
"stxname,\n"
"pg_catalog.pg_get_statisticsobjdef_columns(oid) AS columns,\n"
" 'd' = any(stxkind) AS ndist_enabled,\n"
@@ -2617,7 +2617,7 @@ describeOneTableDetails(const char *schemaname,
"stxstattarget\n"
"FROM pg_catalog.pg_statistic_ext stat\n"
"WHERE stxrelid = '%s'\n"
- "ORDER BY 1;",
+ "ORDER BY nsp, stxname;",
oid);
result = PSQLexec(buf.data);
--
2.17.1
0003-Remove-unused-table-alias.patchtext/x-diff; charset=us-asciiDownload
From 020f425c7ed67183275d64f19c34f69d63b8ddc6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 7 Jan 2022 15:48:46 -0600
Subject: [PATCH 3/3] Remove unused table alias
---
src/bin/psql/describe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index daf06bbcc81..c62d3bd4f8a 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2615,7 +2615,7 @@ describeOneTableDetails(const char *schemaname,
" 'f' = any(stxkind) AS deps_enabled,\n"
" 'm' = any(stxkind) AS mcv_enabled,\n"
"stxstattarget\n"
- "FROM pg_catalog.pg_statistic_ext stat\n"
+ "FROM pg_catalog.pg_statistic_ext\n"
"WHERE stxrelid = '%s'\n"
"ORDER BY nsp, stxname;",
oid);
@@ -2719,7 +2719,7 @@ describeOneTableDetails(const char *schemaname,
appendPQExpBufferStr(&buf, " stxstattarget\n");
else
appendPQExpBufferStr(&buf, " -1 AS stxstattarget\n");
- appendPQExpBuffer(&buf, "FROM pg_catalog.pg_statistic_ext stat\n"
+ appendPQExpBuffer(&buf, "FROM pg_catalog.pg_statistic_ext\n"
"WHERE stxrelid = '%s'\n"
"ORDER BY 1;",
oid);
--
2.17.1
On Fri, Jan 07, 2022 at 03:56:20PM -0600, Justin Pryzby wrote:
I'd propose these.
Applied and backpatched down to 14. One of the aliases is present in
13~, but I have let that alone. The detection regex posted upthread
is kind of cool.
--
Michael
On 2022-Jan-08, Michael Paquier wrote:
The detection regex posted upthread is kind of cool.
Yes, but it's not bulletproof -- it only detects uses of some
unqualified object name that is also used with qualification. Here it
detected "text" unqualified, but only because we already had
pg_catalog.text elsewhere. As an exercise, if you revert this commit
and change one of those "text" to "int", it's not detected as a problem.
My point is that it's good to have it, but it would be much better to
have something bulletproof, which we could use in an automated check
somewhere (next to stuff like perlcritic, perhaps). I don't know what,
though.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Cuando no hay humildad las personas se degradan" (A. Christie)
=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:
My point is that it's good to have it, but it would be much better to
have something bulletproof, which we could use in an automated check
somewhere (next to stuff like perlcritic, perhaps). I don't know what,
though.
Meh ... this is mostly cosmetic these days, so I can't get excited
about putting a lot of work into it. We disclaimed search path
bulletproofness for all these queries a long time ago.
I don't object to fixing it in the name of consistency, but that's
not a reason to invest large effort.
regards, tom lane
Hi justin,
On 2022/01/08 6:56, Justin Pryzby wrote:
On Fri, Jan 07, 2022 at 08:08:57PM +0900, Michael Paquier wrote:
On Fri, Jan 07, 2022 at 06:30:30PM +0900, Tatsuro Yamada wrote:
We should prefix them with pg_catalog as well.
Are you planning to make a patch?
If not, I'll make a patch later since that's where \dX is.If any of you can make a patch, that would be great. Thanks!
I'd propose these.
Thanks!
Regards,
Tatsuro Yamada