pg_dump: Remove trivial usage of PQExpBuffer
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:t52915psql -h localhost -U postgresBuilt from patchset v1 (message #1), July 27, 2026 at 05:40 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 t52915_1 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 t52915_1 && git checkout t52915_1Patchset v1 (message #1) is on t52915_1
I've been looking at ways to reorganize and/or clean up pg_dump.c.
One thing I have noticed is the usage of PQExpBuffer in situations where
the query has no optional parts and no string interpolation.
Attached is a patch to replace those usages with the string literal itself.
There are still a few cases where a buffer is used for a trivial query and
then reset and reused for a more complicated query generation. In those
cases, I did not make the change so as to keep this patch simple.
On 16 Dec 2025, at 23:03, Corey Huinker <corey.huinker@gmail.com> wrote:
One thing I have noticed is the usage of PQExpBuffer in situations where the query has no optional parts and no string interpolation.
- res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+ res = ExecuteSqlQuery(fout,
+ "SELECT DISTINCT attrelid FROM pg_attribute "
+ "WHERE attacl IS NOT NULL",
+ PGRES_TUPLES_OK);I'm not sure I find it an improvement to put have to look after the query text
(which can be long) for the ExecStatusType. Having it separated from the query
is more readable IMHO (I know we have a mix of both already, but I kind of
prefer passing in the buffer).
--
Daniel Gustafsson
On Tue, Dec 16, 2025 at 5:44 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 16 Dec 2025, at 23:03, Corey Huinker <corey.huinker@gmail.com> wrote:
One thing I have noticed is the usage of PQExpBuffer in situations where
the query has no optional parts and no string interpolation.
- res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); + res = ExecuteSqlQuery(fout, + "SELECT DISTINCT attrelid FROM pg_attribute " + "WHERE attacl IS NOT NULL", + PGRES_TUPLES_OK);I'm not sure I find it an improvement to put have to look after the query
text
(which can be long) for the ExecStatusType. Having it separated from the
query
is more readable IMHO (I know we have a mix of both already, but I kind of
prefer passing in the buffer).
I considered replacing them all with the pattern where we assign the block
text to a char *querystr, and in fact that's done in the patch in a couple
of places where the query was an if/else constant. Is that more acceptable?
On 16.12.25 23:03, Corey Huinker wrote:
I've been looking at ways to reorganize and/or clean up pg_dump.c.
One thing I have noticed is the usage of PQExpBuffer in situations where
the query has no optional parts and no string interpolation.Attached is a patch to replace those usages with the string literal
itself.
I'm not sure this is better. It seems better to me to use consistent
APIs throughout. Kind of like using printf even if you don't need to
substitute anything, rather than using a mix of printf and puts.
Hi,
On 2025-12-17 16:15:21 +0100, Peter Eisentraut wrote:
On 16.12.25 23:03, Corey Huinker wrote:
I've been looking at ways to reorganize and/or clean up pg_dump.c.
One thing I have noticed is the usage of PQExpBuffer in situations where
the query has no optional parts and no string interpolation.Attached is a patch to replace those usages with the string literal
itself.I'm not sure this is better. It seems better to me to use consistent APIs
throughout. Kind of like using printf even if you don't need to substitute
anything, rather than using a mix of printf and puts.
It also just seems like a pain for backpatching. If this were a huge
improvement or if the code in question was being newly added, it'd perhaps be
a different story, but as is...
Greetings,
Andres Freund