Make tsqueryout() use a StringInfo
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:t253404psql -h localhost -U postgresBuilt from patchset v1 (message #1), August 19, 2026 at 05:20 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 t253404_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 t253404_1 && git checkout t253404_1Patchset v1 (message #1) is on t253404_1
While doing the recent security work on tsvector/tsquery overflows,
I wondered why tsqueryout() is using its very own hand-rolled
implementation of an extensible string buffer, rather than using
StringInfo like the rest of the backend. I couldn't see any actual
bug there, so changing it was out of scope for a security fix.
But it seems fragile and hard to read, so here's a patch to make it
use StringInfo.
regards, tom lane
Hi,
On Thu, 13 Aug 2026 at 21:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
While doing the recent security work on tsvector/tsquery overflows,
I wondered why tsqueryout() is using its very own hand-rolled
implementation of an extensible string buffer, rather than using
StringInfo like the rest of the backend. I couldn't see any actual
bug there, so changing it was out of scope for a security fix.
But it seems fragile and hard to read, so here's a patch to make it
use StringInfo.
Thanks for the patch!
The patch looks good to me in general. One thing I wonder about is the
increase in the initial allocation: the old code starts with 32 bytes,
whereas initStringInfo() starts with 1024 bytes. ig this can add up when
tsqueryout() is called by array_out(), since array_out() retains each
element's output string while constructing the result?
Would it make sense to use initStringInfoExt(&nrm.buf, 32) here, preserving
the old initial size while retaining automatic growth?
Regards,
Ayush
Ayush Tiwari <ayushtiwari.slg01@gmail.com> writes:
The patch looks good to me in general. One thing I wonder about is the
increase in the initial allocation: the old code starts with 32 bytes,
whereas initStringInfo() starts with 1024 bytes. ig this can add up when
tsqueryout() is called by array_out(), since array_out() retains each
element's output string while constructing the result?
I kinda doubt that a huge array of tsquery's is a realistic scenario.
Would it make sense to use initStringInfoExt(&nrm.buf, 32) here, preserving
the old initial size while retaining automatic growth?
I don't think so. Maybe there's an argument that 1024 is too large,
but I would say that 32 is much too small. Also there are plenty of
other places using the default buffer length without worrying about
this. It seems unlikely to me that quibbling over the value is really
going to be a productive use of brain cells.
regards, tom lane
Hi,
On Sun, 16 Aug 2026 at 23:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ayush Tiwari <ayushtiwari.slg01@gmail.com> writes:
The patch looks good to me in general. One thing I wonder about is the
increase in the initial allocation: the old code starts with 32 bytes,
whereas initStringInfo() starts with 1024 bytes. ig this can add up when
tsqueryout() is called by array_out(), since array_out() retains each
element's output string while constructing the result?I kinda doubt that a huge array of tsquery's is a realistic scenario.
Would it make sense to use initStringInfoExt(&nrm.buf, 32) here,
preserving
the old initial size while retaining automatic growth?
I don't think so. Maybe there's an argument that 1024 is too large,
but I would say that 32 is much too small. Also there are plenty of
other places using the default buffer length without worrying about
this. It seems unlikely to me that quibbling over the value is really
going to be a productive use of brain cells.
Fair enough. I agree this probably isn't worth special-casing without a
realistic workload that demonstrates a problem.
It just seemed a decent bump hence called it out, the rest of the patch
looks good to me.
Regards,
Ayush