Re: Add a semicolon to query related to search_path

Hi Tom,

On 2018/08/15 22:27, Tom Lane wrote:
> Tatsuro Yamada <yamada.tatsuro@lab.ntt.co.jp> 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"

./src/bin/scripts/clusterdb.c:  appendQualifiedRelation(&sql, table, conn, progname, echo);
./src/bin/scripts/common.c:  appendQualifiedRelation(PQExpBuffer buf, const char *spec,
./src/bin/scripts/reindexdb.c:  appendQualifiedRelation(&sql, name, conn, progname, echo);
./src/bin/scripts/vacuumdb.c:   appendQualifiedRelation(sql, table, conn, progname, echo);

Then, I investegated little more and I understand that executeQuery,
ExecuteSqlQueryForSingleRow and executeQueryOrDie call PQexec.

Regards,
Tatsuro Yamada
NTT Open Source Software Center