Обсуждение: pgbench: make verbose error messages thread-safe

Hi,

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

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.

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

On 4/24/26 2:26 PM, Fujii Masao wrote:
> Hi,
>
> 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,
>
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.

So the change looks reasonable and good to me.

Regards,
Alex Guo

> 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.
>
> 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

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
costof writing the log message itself. So I think the simpler fix is probably acceptable. 

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/

On 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
errormessages. In that situation, the cost of one extra memory allocation per log line should be much smaller than the
I/Ocost 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