Make formatting.c use StringInfos for output buffers
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.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
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:t253405psql -h localhost -U postgresBuilt from patchset v1 (message #1), August 20, 2026 at 12:08 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 t253405_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 t253405_1 && git checkout t253405_1Patchset v1 (message #1) is on t253405_1
We've had multiple security bugs (CVE-2026-14669, CVE-2015-0241) in
formatting.c due to its habit of using output buffers of predetermined
length, which are usually much too big but sometimes not big enough.
And the path of least resistance for those security fixes was to
impose arbitrary limits on substring lengths, which is surely a wart.
I think it's time to stop the bleeding once and for all, by switching
that code over to using StringInfos for its output buffers.
That turns out to be a good deal more painful than one could wish,
because the code is messy and inconsistent about how it uses its
buffers; but detangling that seemed like a good code cleanup exercise
anyway. So attached is a patch series to accomplish this:
0001: Convert DCH_to_char() to use a StringInfo output buffer.
0002: Restructure NUM_processor() to make it clearer which
string is the output buffer.
0003: Convert NUM_processor() to use StringInfo output buffers.
0004: Try to buy back some performance (see below).
The main problem with this proposal is that it makes these functions
a little slower, apparently because calling snprintf() via
AppendStringInfo() is slower than calling it directly. After the
performance hacking in 0004, what I see is that float8_to_char
and numeric_to_number are the same speed or a little faster than
before, but timestamptz_to_char is still around 10% slower in
a tight-loop benchmark. (See drive_formatting.c, attached, for
the benchmark infrastructure.) Maybe that's okay given that the
overall effect on a complete SQL query should be far less, but
I'd still like to squeeze out a bit more speed. I don't see any
additional low-hanging fruit though.
regards, tom lane
Attachments:
t253405_1v1-0001-Convert-DCH_to_char-to-use-a-StringInfo-as-destin.patchtext/x-diff; charset=us-ascii; name*0=v1-0001-Convert-DCH_to_char-to-use-a-StringInfo-as-destin.p; name*1=atchDownload+199-357
v1-0002-Restructure-formatting.c-s-NUM_processor-to-add-s.patchtext/x-diff; charset=us-ascii; name*0=v1-0002-Restructure-formatting.c-s-NUM_processor-to-add-s.p; name*1=atchDownload+521-413
v1-0003-Replace-formatting.c-s-fixed-size-output-buffers-.patchtext/x-diff; charset=us-ascii; name*0=v1-0003-Replace-formatting.c-s-fixed-size-output-buffers-.p; name*1=atchDownload+127-184
v1-0004-Micro-optimize-appendStringInfo-VA.patchtext/x-diff; charset=us-ascii; name=v1-0004-Micro-optimize-appendStringInfo-VA.patchDownload+26-14
drive_formatting.c.nocfbottext/x-c; charset=us-ascii; name=drive_formatting.c.nocfbotDownload
On 13/08/2026 20:24, Tom Lane wrote:
We've had multiple security bugs (CVE-2026-14669, CVE-2015-0241) in
formatting.c due to its habit of using output buffers of predetermined
length, which are usually much too big but sometimes not big enough.
And the path of least resistance for those security fixes was to
impose arbitrary limits on substring lengths, which is surely a wart.
I think it's time to stop the bleeding once and for all, by switching
that code over to using StringInfos for its output buffers.That turns out to be a good deal more painful than one could wish,
because the code is messy and inconsistent about how it uses its
buffers; but detangling that seemed like a good code cleanup exercise
anyway. So attached is a patch series to accomplish this:0001: Convert DCH_to_char() to use a StringInfo output buffer.
0002: Restructure NUM_processor() to make it clearer which
string is the output buffer.0003: Convert NUM_processor() to use StringInfo output buffers.
0004: Try to buy back some performance (see below).
This all looks good to me.
The main problem with this proposal is that it makes these functions
a little slower, apparently because calling snprintf() via
AppendStringInfo() is slower than calling it directly. After the
performance hacking in 0004, what I see is that float8_to_char
and numeric_to_number are the same speed or a little faster than
before, but timestamptz_to_char is still around 10% slower in
a tight-loop benchmark. (See drive_formatting.c, attached, for
the benchmark infrastructure.) Maybe that's okay given that the
overall effect on a complete SQL query should be far less, but
I'd still like to squeeze out a bit more speed. I don't see any
additional low-hanging fruit though.
More aggressive inlining seems like the straightforward solution. The
EMIT macros could do more...
You could replace the appendStringInfo() calls with more specialized
functions. Let's take "HH", for example. You could surely have a more
optimized implementation of that than calling appendStringInfo(out,
"%02d", ...).
With some inline functions and macros, you could probably still have it
read DCH_EMITF("%02d", ...) in the source code, but make the DCH_EMITF()
macro check that it's a compile-time constant and route it to a more
efficient function that gets fully inlined at compile time.
Attached is another micro-optimization that makes a surprisingly big
difference on my laptop (10% - 20%). In a nutshell, have a fast-path for
when a constant character in the format string is a one byte character.
I think it's a good bet that most constants in a format string are
characters like spaces, ":" or "-", which are a single ASCII character
in all locales.
I started with a slightly bigger refactoring to replace the
null-terminated FormatNode->character field with a separate length
field. That's also be pretty straightforward, but when I started to test
it, it turns out that you get the same effect from just the attached.
- Heikki
Attachments:
special-case-one-byte-chars.patch.nocfbottext/plain; charset=UTF-8; name=special-case-one-byte-chars.patch.nocfbotDownload+5-1
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 13/08/2026 20:24, Tom Lane wrote:
The main problem with this proposal is that it makes these functions
a little slower, apparently because calling snprintf() via
AppendStringInfo() is slower than calling it directly. After the
performance hacking in 0004, what I see is that float8_to_char
and numeric_to_number are the same speed or a little faster than
before, but timestamptz_to_char is still around 10% slower in
a tight-loop benchmark.
Attached is another micro-optimization that makes a surprisingly big
difference on my laptop (10% - 20%). In a nutshell, have a fast-path for
when a constant character in the format string is a one byte character.
Oh, nice! That fix alone makes timestamptz_to_char faster than it
is on master, which is as far as my ambition extends right now.
I'll proceed with this, and if someone else feels like taking up
your other micro-optimization ideas, feel free.
regards, tom lane