Re: Fix for pg_stat_activity putting client hostaddr into appname field

On Tue, Mar 27, 2018 at 03:47:07PM +1300, Edmund Horner wrote:
> But the stats array includes auxiliary processes, which means it has
> NumBackendStatSlots items. The pointers for the aux process strings
> are out of range. In the case of my query, the pointers for
> st_appname in the aux processes happen to point into the
> BackendClientHostnameBuffer segment.
>
> This patch uses NumBackendStatSlots for the sizes of those segments,
> rather than MaxBackends.

I am adding in CC Robert and Kuntal who worked on that (issue added as
well to the older bug section in v11 open item list).

I have read through your patch and it seems to me that you are right
here. The error comes from the original commit fc70a4b0, which has
added auxiliary processes to pg_stat_activity. Even
BackendStatusShmemSize uses NumBackendStatSlots for the calculation of
the array's lengths, but CreateSharedBackendStatus still mixes it with
NumBackends.

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

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