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
signature.asc
Description: PGP signature
