On Fri, Jan 31, 2014 at 4:40 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-01-30 12:27:43 -0500, Robert Haas wrote:
>> Nope, but I think this patch is broken. It looks to me like it's
>> conflating the process offset in the BackendStatus array with its
>> backendId, which does not seem like a good idea even if it happens to
>> work at present.
>
> Hm. I don't see how that's going to be broken without major surgery in
> pgstat.c. The whole thing seems to rely on being able to index
> BackendStatusArray with MyBackendId?
Oh, you're right. pgstat_initialize() sets it up that way.
>> And the way BackendIdGetProc() is used looks unsafe,
>> too: the contents might no longer be valid by the time we read them.
>> I suspect we should have a new accessor function that takes a backend
>> ID and copies the xid and xmin to pointers provided by the client
>> while holding the lock. I also note that the docs seem to need some
>> copy-editing:
>
> It certainly needs to be documented as racy, but I don't see a big
> problem with being racy here. We assume in lots of places that
> writing/reading xids is atomic, and we don't even hold exclusive locks
> while writing... (And yes, that means that the xid and xmin don't
> necessarily belong to each other)
> That said, encapsulating that racy access into a accessor function does
> sound like a good plan.
Yep, shouldn't be hard to do.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company