Re: Patch: show xid and xmin in pg_stat_activity and pg_stat_replication

On 30 January 2014 17:27, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Jan 29, 2014 at 3:11 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On 14 January 2014 08:38, Christian Kruse <christian@2ndquadrant.com> wrote:
>>> Hi,
>>> On 13/01/14 20:06, Heikki Linnakangas wrote:
>>>> On 12/17/2013 04:58 PM, Christian Kruse wrote:
>>>> >attached you will find a patch for showing the current transaction id
>>>> >(xid) and the xmin of a backend in pg_stat_activty and the xmin in
>>>> >pg_stat_replication.
>>>>
>>>> Docs.
>>>
>>> Thanks, update with updated docs is attached.
>>
>> Looks simple enough and useful for working out which people are
>> holding up CONCURRENT activities.
>>
>> I've not been involved with this patch, so any objections to me doing
>> final review and commit?
>
> 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.  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:
>
> +     <entry>The current <xref linked="ddl-system-columns">xmin
> value.</xref></entry>

Thanks, saved me the trouble of a detailed review... good catches.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services