Redundant strlen(query) in get_rel_infos
Hi.
While reviewing another patch to the file info.c, I noticed there seem
to be some unnecessary calls to strlen(query) in get_rel_infos()
function.
i.e. The query is explicitly initialized to an empty string
immediately prior, so why the strlen?
PSA patch for this.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-Remove-redundant-strlen.patchapplication/octet-stream; name=v1-0001-Remove-redundant-strlen.patchDownload
From ed2dc666ba5b7dd384b8796a47b0e5a81ed5d069 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 11 May 2023 12:59:46 +1000
Subject: [PATCH v1] Remove redundant strlen
---
src/bin/pg_upgrade/info.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index 85ed15a..bf3244f 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -458,7 +458,7 @@ get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo)
* output, so we have to copy that system table. It's easiest to do that
* by treating it as a user table.
*/
- snprintf(query + strlen(query), sizeof(query) - strlen(query),
+ snprintf(query, sizeof(query),
"WITH regular_heap (reloid, indtable, toastheap) AS ( "
" SELECT c.oid, 0::oid, 0::oid "
" FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n "
--
1.8.3.1
On Thu, May 11, 2023 at 01:06:42PM +1000, Peter Smith wrote:
While reviewing another patch to the file info.c, I noticed there seem
to be some unnecessary calls to strlen(query) in get_rel_infos()
function.i.e. The query is explicitly initialized to an empty string
immediately prior, so why the strlen?
It just looks like this was copied from a surrounding area like
get_db_infos(). Keeping the code as it is is no big deal, either, but
yes we could just remove them and save the two calls. So ok by me.
--
Michael
On 11 May 2023, at 06:24, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, May 11, 2023 at 01:06:42PM +1000, Peter Smith wrote:
While reviewing another patch to the file info.c, I noticed there seem
to be some unnecessary calls to strlen(query) in get_rel_infos()
function.i.e. The query is explicitly initialized to an empty string
immediately prior, so why the strlen?It just looks like this was copied from a surrounding area like
get_db_infos(). Keeping the code as it is is no big deal, either, but
yes we could just remove them and save the two calls. So ok by me.
I think it's intentionally done in 73b9952e82 as defensive coding, and given
that this is far from a hot codepath I think leaving them is better.
Instead I think it would be more worthwhile to remove these snprintf() made
queries and use PQExpbuffers. 29aeda6e4e6 introduced that in pg_upgrade and it
is more in line with how we build queries in other tools.
Looking at the snprintf sites made me remember a patchset I worked on last year
(but I don't remember if I ended up submitting); there is no need to build one
of the queries on the stack as it has no variables. The attached 0003 (which
needs a reindent of the query text) comes from that patchset. I think we
should do this regardless.
--
Daniel Gustafsson
Attachments:
0003-pg_upgrade-Avoid-storing-query-on-the-stack.patchapplication/octet-stream; name=0003-pg_upgrade-Avoid-storing-query-on-the-stack.patch; x-unix-mode=0644Download
From 4a5e29522ebe8731b37148873cba39d835e7e0c7 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Mon, 19 Dec 2022 23:29:33 +0100
Subject: [PATCH 3/4] pg_upgrade: Avoid storing query on the stack
There is no use in snprintf'ing the query into a stack buffer since
there are no parameters used. Make the code consistent and hardcode
the query text in the query execution call instead.
---
src/bin/pg_upgrade/tablespace.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/bin/pg_upgrade/tablespace.c b/src/bin/pg_upgrade/tablespace.c
index 69cef7fa6b..f6af6d1be0 100644
--- a/src/bin/pg_upgrade/tablespace.c
+++ b/src/bin/pg_upgrade/tablespace.c
@@ -43,16 +43,13 @@ get_tablespace_paths(void)
PGresult *res;
int tblnum;
int i_spclocation;
- char query[QUERY_ALLOC];
- snprintf(query, sizeof(query),
+ res = executeQueryOrDie(conn,
"SELECT pg_catalog.pg_tablespace_location(oid) AS spclocation "
"FROM pg_catalog.pg_tablespace "
"WHERE spcname != 'pg_default' AND "
" spcname != 'pg_global'");
- res = executeQueryOrDie(conn, "%s", query);
-
if ((os_info.num_old_tablespaces = PQntuples(res)) != 0)
os_info.old_tablespaces =
(char **) pg_malloc(os_info.num_old_tablespaces * sizeof(char *));
--
2.32.1 (Apple Git-133)
On Thu, May 11, 2023 at 11:57:37AM +0200, Daniel Gustafsson wrote:
I think it's intentionally done in 73b9952e82 as defensive coding, and given
that this is far from a hot codepath I think leaving them is better.Instead I think it would be more worthwhile to remove these snprintf() made
queries and use PQExpbuffers. 29aeda6e4e6 introduced that in pg_upgrade and it
is more in line with how we build queries in other tools.
Good idea to reduce the overall presence of QUERY_ALLOC in the
surroundings.
Looking at the snprintf sites made me remember a patchset I worked on last year
(but I don't remember if I ended up submitting); there is no need to build one
of the queries on the stack as it has no variables. The attached 0003 (which
needs a reindent of the query text) comes from that patchset. I think we
should do this regardless.
Not sure that this is an improvement in itself as
get_tablespace_paths() includes QUERY_ALLOC because
executeQueryOrDie() does so, so this could become a problem if
someones decides to copy-paste this code with a query becomes longer
than QUERY_ALLOC once built? Perhaps that's not worth worrying, but I
like your suggestion of applying more PQExpbuffers, particularly if
applied in a consistent way across the board. It could matter if the
code of get_tablespace_paths() is changed to use a query with
parameters.
--
Michael
On 15 May 2023, at 09:45, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, May 11, 2023 at 11:57:37AM +0200, Daniel Gustafsson wrote:
Looking at the snprintf sites made me remember a patchset I worked on last year
(but I don't remember if I ended up submitting); there is no need to build one
of the queries on the stack as it has no variables. The attached 0003 (which
needs a reindent of the query text) comes from that patchset. I think we
should do this regardless.Not sure that this is an improvement in itself as
get_tablespace_paths() includes QUERY_ALLOC because
executeQueryOrDie() does so, so this could become a problem if
someones decides to copy-paste this code with a query becomes longer
than QUERY_ALLOC once built? Perhaps that's not worth worrying,
We already have lots of invocations of executeQueryOrDie which doesn't pass via
a QUERY_ALLOC buffer so I don't see any risk with adding one more.
--
Daniel Gustafsson