On Fri, Jan 31, 2014 at 4:40 AM, Andres Freund <and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
