Avoid repeated scans of the output buffer in oidvectortypes()
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:t254207psql -h localhost -U postgresBuilt from patchset v5 (message #5), October 15, 2026 at 12:28 PM.
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 t254207_5 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 t254207_5 && git checkout t254207_5Patchset v5 (message #5) is on t254207_5
Hi,
oidvectortypes() converts a vector of OIDs to a string of concatenated type names. Since it is typically used to display function argument types, the input vectors are usually short. However, if we pass a long vector, this function has a few issues:
* It uses strcat() to concatenate each type name. strcat() always needs to scan from the beginning of the result string to its end, making this slow for long vectors. We can simply keep a pointer to the end of the result string and append directly there.
* There is no interrupt check in the loop, so cancellation can be delayed for long vectors.
* format_type_extended() returns an allocated string, but that string is not freed after being copied into the result. When the input is short, this is not a problem, we can rely on the memory context to free the memory. But when the vector is large, it is better to free these strings to avoid accumulating unnecessary memory.
To compare the performance:
Before the patch:
```
evantest=# SELECT length(oidvectortypes(repeat('0 ', 400000)::oidvector));
length
---------
1199998
(1 row)
Time: 7364.353 ms (00:07.364)
```
After the patch:
```
evantest=# SELECT length(oidvectortypes(repeat('0 ', 400000)::oidvector));
length
---------
1199998
(1 row)
Time: 20.638 ms
```
Going from 7 seconds to 20 ms is a significant improvement for this large input. Although the benefit in normal use is limited, this seems like a small, worthwhile change.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On 15/10/2026 12:09, Chao Li wrote:
oidvectortypes() converts a vector of OIDs to a string of concatenated type names. Since it is typically used to display function argument types, the input vectors are usually short. However, if we pass a long vector, this function has a few issues:
* It uses strcat() to concatenate each type name. strcat() always needs to scan from the beginning of the result string to its end, making this slow for long vectors. We can simply keep a pointer to the end of the result string and append directly there.
* There is no interrupt check in the loop, so cancellation can be delayed for long vectors.
* format_type_extended() returns an allocated string, but that string is not freed after being copied into the result. When the input is short, this is not a problem, we can rely on the memory context to free the memory. But when the vector is large, it is better to free these strings to avoid accumulating unnecessary memory.
How about switching to a StringInfo?
Going from 7 seconds to 20 ms is a significant improvement for this large input. Although the benefit in normal use is limited, this seems like a small, worthwhile change.
Yep. And aside from the performance, it's always nice to get rid of
strcat() calls. Even though there was no bug here, it's so easy to mess
up the size calculations.
- Heikki
On Thu, 15 Oct 2026 at 12:41, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 15/10/2026 12:09, Chao Li wrote:
oidvectortypes() converts a vector of OIDs to a string of
concatenated type names. Since it is typically used to display
function argument types, the input vectors are usually
short. However, if we pass a long vector, this function has a few
issues:
* It uses strcat() to concatenate each type name. strcat() always
needs to scan from the beginning of the result string to its end,
making this slow for long vectors. We can simply keep a pointer to
the end of the result string and append directly there.
* There is no interrupt check in the loop, so cancellation can be delayed for long vectors.
* format_type_extended() returns an allocated string, but that
string is not freed after being copied into the result. When the
input is short, this is not a problem, we can rely on the memory
context to free the memory. But when the vector is large, it is
better to free these strings to avoid accumulating unnecessary
memory.How about switching to a StringInfo?
I believe the current patch is the smallest correct fix. Introducing StringInfo
would be unnecessary over-engineering.
Going from 7 seconds to 20 ms is a significant improvement for this large input. Although the benefit in normal use is limited, this seems like a small, worthwhile change.
Yep. And aside from the performance, it's always nice to get rid of
strcat() calls. Even though there was no bug here, it's so easy to
mess up the size calculations.
+1
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
On Thu, 15 Oct 2026 at 23:20, Japin Li <japinli@hotmail.com> wrote:
I believe the current patch is the smallest correct fix. Introducing StringInfo
would be unnecessary over-engineering.
I'm assuming this is master only, so why does it need to be a minimal
fix? StringInfo is just the way we normally do incremental building of
strings. It's an API that most people who work on our code understand.
Manually calling repalloc() is tedious and more likely to have bugs.
Using StringInfo will also cut down on the number of reallocs due to
the size doubling. It'll also allow the
PG_RETURN_TEXT_P(cstring_to_text(result)); to be updated to use
cstring_to_text_with_len using the StringInfo's len field.
David
On Oct 15, 2026, at 18:43, David Rowley <dgrowleyml@gmail.com> wrote:
On Thu, 15 Oct 2026 at 23:20, Japin Li <japinli@hotmail.com> wrote:
I believe the current patch is the smallest correct fix. Introducing StringInfo
would be unnecessary over-engineering.I'm assuming this is master only, so why does it need to be a minimal
fix? StringInfo is just the way we normally do incremental building of
strings. It's an API that most people who work on our code understand.
Manually calling repalloc() is tedious and more likely to have bugs.Using StringInfo will also cut down on the number of reallocs due to
the size doubling. It'll also allow the
PG_RETURN_TEXT_P(cstring_to_text(result)); to be updated to use
cstring_to_text_with_len using the StringInfo's len field.David
I tried StringInfo first. A few minor reasons led me to choose the v1 approach:
* v1 made fewer changes to the existing code.
* v1 was slightly faster in my test. With debug and assertions disabled, v1 took about 20 ms and StringInfo about 24 ms, averaged over multiple runs.
I agree that using the usual API outweigh these minor considerations. I've updated the patch to use StringInfo in v2.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Fri, 16 Oct 2026 at 01:16, Chao Li <li.evan.chao@gmail.com> wrote:
I tried StringInfo first. A few minor reasons led me to choose the v1 approach:
* v1 made fewer changes to the existing code.
* v1 was slightly faster in my test. With debug and assertions disabled, v1 took about 20 ms and StringInfo about 24 ms, averaged over multiple runs.
I doubt that matters very much. I think your test case isn't that
realistic anyway, as it uses InvalidOid. I tried exercising it with:
SELECT length(oidvectortypes(repeat('1043 ', 100)::oidvector));
Which gives:
master: tps = 9427.119361
v1: tps = 9684.447542
v2: tps = 9681.868182
v2b: tps = 9707.819189
v2b just replaces the appendStringInfoString(&result, ", "); with
appendBinaryStringInfo(&result, ", ", 2);
I agree that using the usual API outweigh these minor considerations. I've updated the patch to use StringInfo in v2.
v2 looks fine to me.
David
On 15/10/2026 15:49, David Rowley wrote:
On Fri, 16 Oct 2026 at 01:16, Chao Li <li.evan.chao@gmail.com> wrote:
I agree that using the usual API outweigh these minor considerations. I've updated the patch to use StringInfo in v2.
v2 looks fine to me.
Committed, thanks.
- Heikki
On Oct 15, 2026, at 21:52, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 15/10/2026 15:49, David Rowley wrote:
On Fri, 16 Oct 2026 at 01:16, Chao Li <li.evan.chao@gmail.com> wrote:
I agree that using the usual API outweigh these minor considerations. I've updated the patch to use StringInfo in v2.
v2 looks fine to me.
Committed, thanks.
- Heikki
Thanks a lot for pushing.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Oct 15, 2026, at 20:49, David Rowley <dgrowleyml@gmail.com> wrote:
On Fri, 16 Oct 2026 at 01:16, Chao Li <li.evan.chao@gmail.com> wrote:
I tried StringInfo first. A few minor reasons led me to choose the v1 approach:
* v1 made fewer changes to the existing code.
* v1 was slightly faster in my test. With debug and assertions disabled, v1 took about 20 ms and StringInfo about 24 ms, averaged over multiple runs.I doubt that matters very much. I think your test case isn't that
realistic anyway, as it uses InvalidOid. I tried exercising it with:
SELECT length(oidvectortypes(repeat('1043 ', 100)::oidvector));Which gives:
master: tps = 9427.119361
v1: tps = 9684.447542
v2: tps = 9681.868182
v2b: tps = 9707.819189
I intended to avoid the catalog lookup and focus on the difference in string building. But I agree that the real improvement in normal use would not be as significant as my test showed, because valid OIDs require catalog lookups and such long inputs should be rare.
v2b just replaces the appendStringInfoString(&result, ", "); with
appendBinaryStringInfo(&result, ", ", 2);
Good point. appendStringInfoString() calls appendBinaryStringInfo(), so calling appendBinaryStringInfo() directly when the length is known should be better. But since Heikki has already pushed v2, perhaps we can leave it as it is.
I searched the source tree and found several other places where we can replace appendStringInfoString() with appendBinaryStringInfo(), for example in ExplainFlushWorkersState():
```
for (int i = 0; i < wstate->num_workers; i++)
{
if (wstate->worker_inited[i])
{
/* This must match previous ExplainOpenSetAsideGroup call */
ExplainOpenGroup("Worker", NULL, true, es);
appendStringInfoString(es->str, wstate->worker_str[i].data);
ExplainCloseGroup("Worker", NULL, true, es);
pfree(wstate->worker_str[i].data);
}
}
```
Since the length is already available, we can replace the appendStringInfoString() call with: appendBinaryStringInfo(es->str, wstate->worker_str[i].data, wstate->worker_str[i].len);
If this is worth doing, I can prepare a separate patch.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/