Redundant strlen(query) in get_rel_infos
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t47843psql -h localhost -U postgresBuilt from patchset v3 (message #3), July 28, 2026 at 12:41 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t47843_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t47843_3 && git checkout t47843_3Patchset v3 (message #3) is on t47843_3
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+1-2
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
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