On 29/03/18 10:46, Michael Paquier wrote:
> On Tue, Mar 27, 2018 at 03:47:07PM +1300, Edmund Horner wrote:
>> I considered whether aux processes really need those strings
>> (especially st_clienthostname), but decided it was more consistent
>> just to assume that they might. (It's an extra 3 kB... if we want to
>> save that we can put a bunch of if statements in pgstat_bestart and
>> other places.)
>
> BackendStatusShmemSize has been originally changed to use
> NumBackendStatSlots, so the intention is visibly to be consistent in the
> number of backends used, even if this means consuming a bit more memory,
> so the idea was to focus on consistency and simplicity.
Yep, agreed.
I've committed this, and backpatched to v10.
>> The patch also allocates local memory for st_clienthostname in
>> pgstat_read_current_status. These strings shouldn't change very
>> often, but it seems safer and more consistent to treat them as we
>> treat the other two string fields. Without the string copy, I think a
>> client disconnect and be replaced after the stats have been copied,
>> resulting in the new hostname appearing alongside the copied stats
>> fields from the old connection.
>
> Agreed on that as well.
Yeah, that's also a bug. I pushed your fix for that as a separate
commit, and backpatched to all supported versions.
Thanks for the debugging and the patch, Edmund!
- Heikki
