pgsql-hackers
❮
pgbench: make verbose error messages thread-safe
- Jump to comment-1Fujii Masao<masao.fujii@gmail.com>Apr 24, 2026, 6:26 AM UTCHi,
While running pgbench with multiple threads and --verbose-errors,
I found that some verbose error messages were corrupted.
This issue happens because printVerboseErrorMessages() uses
a function-local static PQExpBuffer for building those messages.
Since that buffer is shared across threads, it is not thread-safe.
Attached patch fixes this issue by changing printVerboseErrorMessages()
to use a local PQExpBufferData instead of a static one. Thoughts?
Since this issue was introduced in v15, the patch should be
backpatched to v15 if accepted.
Regards,
--
Fujii Masao- Jump to comment-1Michael Paquier<michael@paquier.xyz>Apr 24, 2026, 8:07 AM UTCOn Fri, Apr 24, 2026 at 03:26:03PM +0900, Fujii Masao wrote:
Attached patch fixes this issue by changing printVerboseErrorMessages()
That looks like an oversight of 4a39f87acd6e to me. A static buffer
to use a local PQExpBufferData instead of a static one. Thoughts?
in this context is not adapted.Since this issue was introduced in v15, the patch should be
This forces a new allocation for each message printed vs a set of
backpatched to v15 if accepted.
resets after one allocation is done. This change is not going to be
entirely free as done in the patch, so should we worry about that?
Perhaps it would be cheaper to allocate a PQExpBuffer in each CState,
and just reuse it in this routine?
--
Michael- Jump to comment-1Chao Li<li.evan.chao@gmail.com>Apr 24, 2026, 9:06 AM UTC
On Apr 24, 2026, at 16:07, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Apr 24, 2026 at 03:26:03PM +0900, Fujii Masao wrote:Attached patch fixes this issue by changing printVerboseErrorMessages()
to use a local PQExpBufferData instead of a static one. Thoughts?
That looks like an oversight of 4a39f87acd6e to me. A static buffer
in this context is not adapted.Since this issue was introduced in v15, the patch should be
backpatched to v15 if accepted.
I am not too worried about that, because this code path is only used with --verbose-errors and only when printing error messages. In that situation, the cost of one extra memory allocation per log line should be much smaller than the I/O cost of writing the log message itself. So I think the simpler fix is probably acceptable.
This forces a new allocation for each message printed vs a set of
resets after one allocation is done. This change is not going to be
entirely free as done in the patch, so should we worry about that?
Perhaps it would be cheaper to allocate a PQExpBuffer in each CState,
and just reuse it in this routine?
--
Michael
But if we really care about the performance problem, I think PQExpBuffer might be better stored in TState that is per thread, while CState is per connection.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/- Jump to comment-1Fujii Masao<masao.fujii@gmail.com>Apr 24, 2026, 11:24 AM UTCOn Fri, Apr 24, 2026 at 6:06 PM Chao Li <li.evan.chao@gmail.com> wrote:
I am not too worried about that, because this code path is only used with --verbose-errors and only when printing error messages. In that situation, the cost of one extra memory allocation per log line should be much smaller than the I/O cost of writing the log message itself. So I think the simpler fix is probably acceptable.
Yes, I feel the same way.But if we really care about the performance problem, I think PQExpBuffer might be better stored in TState that is per thread, while CState is per connection.
Agreed.
Regards,
--
Fujii Masao
- Jump to comment-1Alex Guo<guo.alex.hengchen@gmail.com>Apr 24, 2026, 8:09 AM UTCOn 4/24/26 2:26 PM, Fujii Masao wrote:
Hi,
In single-threaded mode, reusing the static local buffer might be slightly more performant. But if this function needs to be safe in multi-threaded mode, then we have to stop using a static local variable.
While running pgbench with multiple threads and --verbose-errors,
I found that some verbose error messages were corrupted.
This issue happens because printVerboseErrorMessages() uses
a function-local static PQExpBuffer for building those messages.
Since that buffer is shared across threads, it is not thread-safe.
Attached patch fixes this issue by changing printVerboseErrorMessages()
to use a local PQExpBufferData instead of a static one. Thoughts?
Since this issue was introduced in v15, the patch should be
backpatched to v15 if accepted.
Regards,
So the change looks reasonable and good to me.
Regards,
Alex Guo