[Tom Lane <tgl@sss.pgh.pa.us>]

While investigating for 353708e1f, I happened to notice that pg_dump's
binary_upgrade_set_type_oids_by_type_oid() contains

    PQExpBuffer upgrade_query = createPQExpBuffer();
    ...
    appendPQExpBuffer(upgrade_query,
                      "SELECT typarray "
    ...
    res = ExecuteSqlQueryForSingleRow(fout, upgrade_query->data);
    ...
            appendPQExpBuffer(upgrade_query,
                              "SELECT t.oid, t.typarray "
    ...
            res = ExecuteSqlQueryForSingleRow(fout, upgrade_query->data);

How's that work?  It turns out that the second ExecuteSqlQueryForSingleRow
is sending a string like "SELECT typarray ...;SELECT t.oid, t.typarray ..."
which the backend happily interprets as multiple commands.  It sends
back multiple query results, and then PQexec discards all but the last
one.  So, very accidentally, there's no observable bug, just some wasted
cycles.

I will go fix this, but I wondered if there are any other similar
errors, or what we might do to prevent the same type of mistake
in future.  I experimented with replacing most of pg_dump's PQexec
calls with PQexecParams, as in the attached quick hack (NOT meant
for commit).  That did not turn up any additional cases, but of
course I have limited faith in the code coverage of check-world.

We could consider a more global change to get rid of using
appendPQExpBuffer where it's not absolutely necessary, so that
there are fewer bad examples to copy.  Another idea is to deem
it an anti-pattern to end a query with a semicolon.  But I don't
have a lot of faith in people following those coding rules in
future, either.  It'd also be a lot of code churn for what is
in the end a relatively harmless bug.

Thoughts?

			regards, tom lane

Attachments:

  [text/x-diff] hacky-check-for-bogus-pg_dump-queries.patch (4.3K, ../1714711.1642962663@sss.pgh.pa.us/2-hacky-check-for-bogus-pg_dump-queries.patch)
  download | inline diff:
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index bc5251be82..b48a679faf 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -1320,7 +1320,9 @@ lockTableForWorker(ArchiveHandle *AH, TocEntry *te)
 	appendPQExpBuffer(query, "LOCK TABLE %s IN ACCESS SHARE MODE NOWAIT",
 					  qualId);
 
-	res = PQexec(AH->connection, query->data);
+	res = PQexecParams(AH->connection, query->data,
+					   0, NULL, NULL, NULL,
+					   NULL, 0);
 
 	if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
 		fatal("could not obtain lock on relation \"%s\"\n"
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 49bf0907cd..85e6fe6523 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3122,7 +3122,9 @@ _doSetSessionAuth(ArchiveHandle *AH, const char *user)
 	{
 		PGresult   *res;
 
-		res = PQexec(AH->connection, cmd->data);
+		res = PQexecParams(AH->connection, cmd->data,
+						   0, NULL, NULL, NULL,
+						   NULL, 0);
 
 		if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
 			/* NOT warn_or_exit_horribly... use -O instead to skip this. */
@@ -3259,7 +3261,9 @@ _selectOutputSchema(ArchiveHandle *AH, const char *schemaName)
 	{
 		PGresult   *res;
 
-		res = PQexec(AH->connection, qry->data);
+		res = PQexecParams(AH->connection, qry->data,
+						   0, NULL, NULL, NULL,
+						   NULL, 0);
 
 		if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
 			warn_or_exit_horribly(AH,
@@ -3321,7 +3325,9 @@ _selectTablespace(ArchiveHandle *AH, const char *tablespace)
 	{
 		PGresult   *res;
 
-		res = PQexec(AH->connection, qry->data);
+		res = PQexecParams(AH->connection, qry->data,
+						   0, NULL, NULL, NULL,
+						   NULL, 0);
 
 		if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
 			warn_or_exit_horribly(AH,
@@ -3371,7 +3377,9 @@ _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam)
 	{
 		PGresult   *res;
 
-		res = PQexec(AH->connection, cmd->data);
+		res = PQexecParams(AH->connection, cmd->data,
+						   0, NULL, NULL, NULL,
+						   NULL, 0);
 
 		if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
 			warn_or_exit_horribly(AH,
diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c
index 3184eda3e7..b621f43933 100644
--- a/src/bin/pg_dump/pg_backup_db.c
+++ b/src/bin/pg_dump/pg_backup_db.c
@@ -279,7 +279,9 @@ ExecuteSqlStatement(Archive *AHX, const char *query)
 	ArchiveHandle *AH = (ArchiveHandle *) AHX;
 	PGresult   *res;
 
-	res = PQexec(AH->connection, query);
+	res = PQexecParams(AH->connection, query,
+					   0, NULL, NULL, NULL,
+					   NULL, 0);
 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
 		die_on_query_failure(AH, query);
 	PQclear(res);
@@ -291,7 +293,9 @@ ExecuteSqlQuery(Archive *AHX, const char *query, ExecStatusType status)
 	ArchiveHandle *AH = (ArchiveHandle *) AHX;
 	PGresult   *res;
 
-	res = PQexec(AH->connection, query);
+	res = PQexecParams(AH->connection, query,
+					   0, NULL, NULL, NULL,
+					   NULL, 0);
 	if (PQresultStatus(res) != status)
 		die_on_query_failure(AH, query);
 	return res;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7c0e396ce1..f1e8b0b5c2 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4574,7 +4574,7 @@ binary_upgrade_set_type_oids_by_type_oid(Archive *fout,
 	{
 		if (fout->remoteVersion >= 140000)
 		{
-			appendPQExpBuffer(upgrade_query,
+			printfPQExpBuffer(upgrade_query,
 							  "SELECT t.oid, t.typarray "
 							  "FROM pg_catalog.pg_type t "
 							  "JOIN pg_catalog.pg_range r "
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 10383c713f..cfe97bf530 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -1667,7 +1667,9 @@ executeQuery(PGconn *conn, const char *query)
 
 	pg_log_info("executing %s", query);
 
-	res = PQexec(conn, query);
+	res = PQexecParams(conn, query,
+					   0, NULL, NULL, NULL,
+					   NULL, 0);
 	if (!res ||
 		PQresultStatus(res) != PGRES_TUPLES_OK)
 	{
@@ -1690,7 +1692,9 @@ executeCommand(PGconn *conn, const char *query)
 
 	pg_log_info("executing %s", query);
 
-	res = PQexec(conn, query);
+	res = PQexecParams(conn, query,
+					   0, NULL, NULL, NULL,
+					   NULL, 0);
 	if (!res ||
 		PQresultStatus(res) != PGRES_COMMAND_OK)
 	{