Clean up command argument assembly
This is a small code cleanup patch.
Several commands internally assemble command lines to call other
commands. This includes initdb, pg_dumpall, and pg_regress. (Also
pg_ctl, but that is different enough that I didn't consider it here.)
This has all evolved a bit organically, with fixed-size buffers, and
various optional command-line arguments being injected with
confusing-looking code, and the spacing between options handled in
inconsistent ways. This patch cleans all this up a bit to look clearer
and be more easily extensible with new arguments and options. We start
each command with printfPQExpBuffer(), and then append arguments as
necessary with appendPQExpBuffer(). Also standardize on using
initPQExpBuffer() over createPQExpBuffer() where possible. pg_regress
uses StringInfo instead of PQExpBuffer, but many of the same ideas apply.
Attachments:
0001-Clean-up-command-argument-assembly.patchtext/plain; charset=UTF-8; name=0001-Clean-up-command-argument-assembly.patchDownload+62-48
On 26/06/2023 12:33, Peter Eisentraut wrote:
This is a small code cleanup patch.
Several commands internally assemble command lines to call other
commands. This includes initdb, pg_dumpall, and pg_regress. (Also
pg_ctl, but that is different enough that I didn't consider it here.)
This has all evolved a bit organically, with fixed-size buffers, and
various optional command-line arguments being injected with
confusing-looking code, and the spacing between options handled in
inconsistent ways. This patch cleans all this up a bit to look clearer
and be more easily extensible with new arguments and options.
+1
We start each command with printfPQExpBuffer(), and then append
arguments as necessary with appendPQExpBuffer(). Also standardize on
using initPQExpBuffer() over createPQExpBuffer() where possible.
pg_regress uses StringInfo instead of PQExpBuffer, but many of the
same ideas apply.
It's a bit bogus to use PQExpBuffer for these. If you run out of memory,
you silently get an empty string instead. StringInfo, which exits the
process on OOM, would be more appropriate. We have tons of such
inappropriate uses of PQExpBuffer in all our client programs, though, so
I don't insist on fixing this particular case right now.
--
Heikki Linnakangas
Neon (https://neon.tech)
On 04.07.23 14:14, Heikki Linnakangas wrote:
On 26/06/2023 12:33, Peter Eisentraut wrote:
This is a small code cleanup patch.
Several commands internally assemble command lines to call other
commands. This includes initdb, pg_dumpall, and pg_regress. (Also
pg_ctl, but that is different enough that I didn't consider it here.)
This has all evolved a bit organically, with fixed-size buffers, and
various optional command-line arguments being injected with
confusing-looking code, and the spacing between options handled in
inconsistent ways. This patch cleans all this up a bit to look clearer
and be more easily extensible with new arguments and options.+1
committed
We start each command with printfPQExpBuffer(), and then append
arguments as necessary with appendPQExpBuffer(). Also standardize on
using initPQExpBuffer() over createPQExpBuffer() where possible.
pg_regress uses StringInfo instead of PQExpBuffer, but many of the
same ideas apply.It's a bit bogus to use PQExpBuffer for these. If you run out of memory,
you silently get an empty string instead. StringInfo, which exits the
process on OOM, would be more appropriate. We have tons of such
inappropriate uses of PQExpBuffer in all our client programs, though, so
I don't insist on fixing this particular case right now.
Interesting point. But as you say better dealt with as a separate problem.
On 05.07.23 07:22, Peter Eisentraut wrote:
It's a bit bogus to use PQExpBuffer for these. If you run out of
memory, you silently get an empty string instead. StringInfo, which
exits the process on OOM, would be more appropriate. We have tons of
such inappropriate uses of PQExpBuffer in all our client programs,
though, so I don't insist on fixing this particular case right now.Interesting point. But as you say better dealt with as a separate problem.
I was inspired by a33e17f210 (for pg_rewind) to clean up some more
fixed-buffer command assembly and replace it with extensible buffers and
some more elegant code. And then I remembered this thread, and it's
really a continuation of this.
The first patch deals with pg_regress and pg_isolation_regress. It is
pretty straightforward.
The second patch deals with pg_upgrade. It would require exporting
appendPQExpBufferVA() from libpq, which might be overkill. But this
gets to your point earlier: Should pg_upgrade rather be using
StringInfo instead of PQExpBuffer? (Then we'd use appendStringInfoVA(),
which already exists, but even if not we wouldn't need to change libpq
to get it.) Should anything outside of libpq be using PQExpBuffer?
Attachments:
0001-Use-extensible-buffers-to-assemble-command-lines.patchtext/plain; charset=UTF-8; name=0001-Use-extensible-buffers-to-assemble-command-lines.patchDownload+30-49
0002-WIP-pg_upgrade-No-more-command-too-long.patchtext/plain; charset=UTF-8; name=0002-WIP-pg_upgrade-No-more-command-too-long.patchDownload+19-20
Peter Eisentraut <peter@eisentraut.org> writes:
Should anything outside of libpq be using PQExpBuffer?
Perhaps not. PQExpBuffer's behavior for OOM cases is designed
specifically for libpq, where exit-on-OOM is not okay and we
can hope to include failure checks wherever needed. For most
of our application code, we'd much rather just exit-on-OOM
and not have to think about failure checks at the call sites.
Having said that, converting stuff like pg_dump would be quite awful
in terms of code churn and creating a back-patching nightmare.
Would it make any sense to think about having two sets of
routines with identical call APIs, but different failure
behavior, so that we don't have to touch the callers?
regards, tom lane