Re: Add a semicolon to query related to search_path
On 2018/08/31 2:28, Peter Eisentraut wrote: On 17/08/2018 05:32, Tatsuro Yamada wrote: Hi Robert, On 2018/08/17 4:32, Robert Haas wrote: On Thu, Aug 16, 2018 at 1:20 AM, Tatsuro Yamada wrote: As you can see, queries with and without a semicolon are mixed, it is hard to understand the end of each query. This is not beautiful and user-friendly, do not you think so? I agree with you. Thanks! This fix is not a noteworthy new feature, but I believe it will be useful for users to improve readability. I'll add the patch to commitfest. committed Hi Peter, Thank you! Regards, Tatsuro Yamada
Re: Add a semicolon to query related to search_path
On 17/08/2018 05:32, Tatsuro Yamada wrote: > Hi Robert, > > On 2018/08/17 4:32, Robert Haas wrote: >> On Thu, Aug 16, 2018 at 1:20 AM, Tatsuro Yamada >> wrote: >>> As you can see, queries with and without a semicolon are mixed, it is hard >>> to understand the end of each query. This is not beautiful and >>> user-friendly, >>> do not you think so? >> >> I agree with you. > > Thanks! > This fix is not a noteworthy new feature, but I believe it will be useful for > users to improve readability. I'll add the patch to commitfest. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Add a semicolon to query related to search_path
Hi Robert, On 2018/08/17 4:32, Robert Haas wrote: On Thu, Aug 16, 2018 at 1:20 AM, Tatsuro Yamada wrote: As you can see, queries with and without a semicolon are mixed, it is hard to understand the end of each query. This is not beautiful and user-friendly, do not you think so? I agree with you. Thanks! This fix is not a noteworthy new feature, but I believe it will be useful for users to improve readability. I'll add the patch to commitfest. Regards, Tatsuro Yamada NTT Open Source Software Center
Re: Add a semicolon to query related to search_path
On Thu, Aug 16, 2018 at 1:20 AM, Tatsuro Yamada wrote: > As you can see, queries with and without a semicolon are mixed, it is hard > to understand the end of each query. This is not beautiful and > user-friendly, > do not you think so? I agree with you. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Add a semicolon to query related to search_path
Hi Tom,
On 2018/08/15 22:27, Tom Lane wrote:
Tatsuro Yamada writes:
Attached patch gives the following query a semicolon for readability.
s/SELECT pg_catalog.set_config ('search_path', '', false)/
SELECT pg_catalog.set_config ('search_path', '', false);/
I'm not exactly convinced that this is worth doing. There are an
awful lot of queries issued by our various client tools, and there's
basically no consistency as to whether they use trailing semicolons
or not. I do not think it is a useful exercise to try to impose
such consistency, even assuming that we could settle on which way
is better. (I do not necessarily buy your assumption that
with-semicolon is better.)
A concrete reason not to do that is that if we only ever test one
way, we might accidentally break the backend's handling of the
other way. Admittedly, we'd have to get close to 100% consistency
before that became a serious hazard, but what's the point of moving
the needle just a little bit?
Thanks for your comment.
I understand that those queries are issued from various client tools, and
that they do not care about semicolons.
In the test as you wrote, I think that it is a another story that the
processing of the other side may be broken by aligning it to either the
presence or absence of the semicolon of the query. The problem depends on
PQExec's regression test and should be added to the regression test with
semicolon presence or absence.
The answer why do you want to fix is I want to make client tool's output
user-friendly. I have only focused on "clusterdb, vacuumdb and reindexdb"
because they have "-e/--echo" option to output the queries. See the
results of the following three commands:
$ clusterdb -t foo -e
SELECT pg_catalog.set_config('search_path', '', false)
RESET search_path
SELECT c.relname, ns.nspname
FROM pg_catalog.pg_class c, pg_catalog.pg_namespace ns
WHERE c.relnamespace OPERATOR(pg_catalog.=) ns.oid
AND c.oid OPERATOR(pg_catalog.=) 'foo'::pg_catalog.regclass;
SELECT pg_catalog.set_config('search_path', '', false)
CLUSTER public.foo;
$ vacuumdb -t foo -e
SELECT pg_catalog.set_config('search_path', '', false)
vacuumdb: vacuuming database "postgres"
RESET search_path
SELECT c.relname, ns.nspname
FROM pg_catalog.pg_class c, pg_catalog.pg_namespace ns
WHERE c.relnamespace OPERATOR(pg_catalog.=) ns.oid
AND c.oid OPERATOR(pg_catalog.=) 'foo'::pg_catalog.regclass;
SELECT pg_catalog.set_config('search_path', '', false)
VACUUM public.foo;
$ reindexdb -t foo -e
SELECT pg_catalog.set_config('search_path', '', false)
RESET search_path
SELECT c.relname, ns.nspname
FROM pg_catalog.pg_class c, pg_catalog.pg_namespace ns
WHERE c.relnamespace OPERATOR(pg_catalog.=) ns.oid
AND c.oid OPERATOR(pg_catalog.=) 'foo'::pg_catalog.regclass;
SELECT pg_catalog.set_config('search_path', '', false)
REINDEX TABLE public.foo;
As you can see, queries with and without a semicolon are mixed, it is hard
to understand the end of each query. This is not beautiful and user-friendly,
do not you think so? This patch is intended to make it easier for users to
read the output from the client tools (clusterdb, vacuumdb and reindexdb).
The following result is investigation of the function affected by the patch.
- ALWAYS_SECURE_SEARCH_PATH_SQL @fe_utils/connect.h is "SELECT
pg_catalog.set_config ('search_path', '', false)".
- appendQualifiedRelation @src/bin/scripts/common.c includes "executeCommand(conn,
"RESET search_path", progname, echo);"
$ find . -type f -name "*.c" | xargs grep "ALWAYS_SECURE_SEARCH_PATH_SQL"
./contrib/oid2name/oid2name.c: res = PQexec(conn,
ALWAYS_SECURE_SEARCH_PATH_SQL);
./contrib/vacuumlo/vacuumlo.c: res = PQexec(conn,
ALWAYS_SECURE_SEARCH_PATH_SQL);
./src/bin/pg_basebackup/streamutil.c: res = PQexec(tmpconn,
ALWAYS_SECURE_SEARCH_PATH_SQL);
./src/bin/pg_dump/pg_backup_db.c: ALWAYS_SECURE_SEARCH_PATH_SQL));
./src/bin/pg_dump/pg_backup_db.c: ALWAYS_SECURE_SEARCH_PATH_SQL));
./src/bin/pg_dump/pg_dumpall.c: PQclear(executeQuery(conn,
ALWAYS_SECURE_SEARCH_PATH_SQL));
./src/bin/pg_dump/pg_dump.c:PQclear(ExecuteSqlQueryForSingleRow(AH,
ALWAYS_SECURE_SEARCH_PATH_SQL));
./src/bin/pg_dump/pg_dump.c: ALWAYS_SECURE_SEARCH_PATH_SQL));
./src/bin/pg_rewind/libpq_fetch.c: res = PQexec(conn,
ALWAYS_SECURE_SEARCH_PATH_SQL);
./src/bin/pg_upgrade/server.c: PQclear(executeQueryOrDie(conn,
ALWAYS_SECURE_SEARCH_PATH_SQL));
./src/bin/scripts/common.c: PQclear(executeQuery(conn,
ALWAYS_SECURE_SEARCH_PATH_SQL,
./src/bin/scripts/common.c: PQclear(executeQuery(conn,
ALWAYS_SECURE_SEARCH_PATH_SQL,
./src/tools/findoidjoins/findoidjoins.c: res = PQexec(conn,
ALWAYS_SECURE_SEARCH_PATH_SQL);
$ find . -type f -name "*.c" | xargs grep "appendQualifiedRelation"
Re: Add a semicolon to query related to search_path
Tatsuro Yamada writes:
> Attached patch gives the following query a semicolon for readability.
>s/SELECT pg_catalog.set_config ('search_path', '', false)/
> SELECT pg_catalog.set_config ('search_path', '', false);/
I'm not exactly convinced that this is worth doing. There are an
awful lot of queries issued by our various client tools, and there's
basically no consistency as to whether they use trailing semicolons
or not. I do not think it is a useful exercise to try to impose
such consistency, even assuming that we could settle on which way
is better. (I do not necessarily buy your assumption that
with-semicolon is better.)
A concrete reason not to do that is that if we only ever test one
way, we might accidentally break the backend's handling of the
other way. Admittedly, we'd have to get close to 100% consistency
before that became a serious hazard, but what's the point of moving
the needle just a little bit?
regards, tom lane
Add a semicolon to query related to search_path
Hi,
I found some improvements in Client Applications in /src/bin/scripts when I
resumed development of progress monitor for cluster command.
Attached patch gives the following query a semicolon for readability.
s/SELECT pg_catalog.set_config ('search_path', '', false)/
SELECT pg_catalog.set_config ('search_path', '', false);/
s/RESET search_path/RESET search_path;/
For example,
Client application vacuumdb's results using the patch are following:
# Not patched #
$ vacuumdb -e -Zt 'pg_am(amname)'
SELECT pg_catalog.set_config ('search_path', '', false)
vacuumdb: vacuuming database "postgres"
RESET search_path
SELECT c.relname, ns.nspname
FROM pg_catalog.pg_class c, pg_catalog.pg_namespace ns
WHERE c.relnamespace OPERATOR (pg_catalog. =) Ns.oid
AND c.oid OPERATOR (pg_catalog. =) 'Pg_am' :: pg_catalog.regclass;
SELECT pg_catalog.set_config ('search_path', '', false)
ANALYZE pg_catalog.pg_am (amname);
# Patched #
$ vacuumdb -e -Zt 'pg_am(amname)'
SELECT pg_catalog.set_config ('search_path', '', false);
vacuumdb: vacuuming database "postgres"
RESET search_path;
SELECT c.relname, ns.nspname
FROM pg_catalog.pg_class c, pg_catalog.pg_namespace ns
WHERE c.relnamespace OPERATOR (pg_catalog. =) Ns.oid
AND c.oid OPERATOR (pg_catalog. =) 'Pg_am' :: pg_catalog.regclass;
SELECT pg_catalog.set_config ('search_path', '', false);
ANALYZE pg_catalog.pg_am (amname);
I tested "make check-world" and "make installcheck-world" on 777e6ddf1
and are fine.
Regards,
Tatsuro Yamada
NTT Open Source Software Center
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index db2b9f0d68..a80089ccde 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -335,7 +335,7 @@ appendQualifiedRelation(PQExpBuffer buf, const char *spec,
appendStringLiteralConn(, table, conn);
appendPQExpBufferStr(, "::pg_catalog.regclass;");
- executeCommand(conn, "RESET search_path", progname, echo);
+ executeCommand(conn, "RESET search_path;", progname, echo);
/*
* One row is a typical result, as is a nonexistent relation ERROR.
diff --git a/src/include/fe_utils/connect.h b/src/include/fe_utils/connect.h
index fa293d2458..d62f5a3724 100644
--- a/src/include/fe_utils/connect.h
+++ b/src/include/fe_utils/connect.h
@@ -23,6 +23,6 @@
* might work with the old server, skip this.
*/
#define ALWAYS_SECURE_SEARCH_PATH_SQL \
- "SELECT pg_catalog.set_config('search_path', '', false)"
+ "SELECT pg_catalog.set_config('search_path', '', false);"
#endif /* CONNECT_H */
