Re: Patch: show xid and xmin in pg_stat_activity and pg_stat_replication - Mailing list pgsql-hackers
From | Andres Freund |
---|---|
Subject | Re: Patch: show xid and xmin in pg_stat_activity and pg_stat_replication |
Date | |
Msg-id | 20140201162306.GE32407@alap3.anarazel.de Whole thread Raw |
In response to | Re: Patch: show xid and xmin in pg_stat_activity and pg_stat_replication (Christian Kruse <christian@2ndquadrant.com>) |
Responses |
Re: Patch: show xid and xmin in pg_stat_activity and
pg_stat_replication
|
List | pgsql-hackers |
Him On 2014-02-01 17:03:46 +0100, Christian Kruse wrote: > diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml > index a37e6b6..bef5912 100644 > --- a/doc/src/sgml/monitoring.sgml > +++ b/doc/src/sgml/monitoring.sgml > @@ -629,6 +629,16 @@ postgres: <replaceable>user</> <replaceable>database</> <replaceable>host</> <re > </entry> > </row> > <row> > + <entry><structfield>transactionid</structfield></entry> > + <entry><type>xid</type></entry> > + <entry>The current transaction identifier.</entry> > + </row> The other entries refer to the current backend. Maybe Toplevel transaction identifier of this backend, if any. > + <row> > + <entry><structfield>xmin</structfield></entry> > + <entry><type>xid</type></entry> > + <entry>The current <xref linked="ddl-system-columns">xmin</xref> value.</entry> > + </row> > + <row> I don't think that link is correct, the xmin you're linking to is about a row's xmin, while the column you're documenting is the backends current xmin horizon. Maybe: The current backend's xmin horizon. > <entry><structfield>query</></entry> > <entry><type>text</></entry> > <entry>Text of this backend's most recent query. If > @@ -1484,6 +1494,11 @@ postgres: <replaceable>user</> <replaceable>database</> <replaceable>host</> <re > </entry> > </row> > <row> > + <entry><structfield>xmin</structfield></entry> > + <entry><type>xid</type></entry> > + <entry>The current <xref linked="ddl-system-columns">xmin value.</xref></entry> > + </row> > + <row> Wrong link again. This should probably read "This standby's xmin horizon reported by hot_standby_feedback." > @@ -2785,6 +2787,8 @@ pgstat_read_current_status(void) > volatile PgBackendStatus *beentry; > PgBackendStatus *localtable; > PgBackendStatus *localentry; > + PGPROC *proc; > + PGXACT *xact; A bit hard to read from the diff only, but aren't they now unused? > char *localappname, > *localactivity; > int i; > @@ -2848,6 +2852,8 @@ pgstat_read_current_status(void) > /* Only valid entries get included into the local array */ > if (localentry->st_procpid > 0) > { > + BackendIdGetTransactionIds(localentry->st_procpid, &localentry->transactionid, &localentry->xmin); > + That's a bit of a long line, try to keep it to 79 chars. > /* > + * BackendIdGetTransactionIds > + * Get the PGPROC structure for a backend, given the backend ID. Also > + * get the xid and xmin of the backend. The result may be out of date > + * arbitrarily quickly, so the caller must be careful about how this > + * information is used. NULL is returned if the backend is not active. > + */ > +PGPROC * > +BackendIdGetTransactionIds(int backendPid, TransactionId *xid, TransactionId *xmin) > +{ Hm, why do you even return the PGPROC here? Not that's problematic, but it seems a bit pointless. If you remove it you can remove a fair bit of the documentation. I think it should note instead that the two values can be out of whack with each other. > + PGPROC *result = NULL; > + ProcState *stateP; > + SISeg *segP = shmInvalBuffer; > + int index = 0; > + PGXACT *xact; > + > + /* Need to lock out additions/removals of backends */ > + LWLockAcquire(SInvalWriteLock, LW_SHARED); > + > + if (backendPid > 0) > + { > + for (index = 0; index < segP->lastBackend; index++) > + { > + if (segP->procState[index].procPid == backendPid) > + { > + stateP = &segP->procState[index]; > + result = stateP->proc; > + xact = &ProcGlobal->allPgXact[result->pgprocno]; > + > + *xid = xact->xid; > + *xmin = xact->xmin; > + > + break; > + } > + } > + } > + > + LWLockRelease(SInvalWriteLock); > + > + return result; > +} Uh, why do we suddenly need the loop here? BackendIdGetProc() works without one, so this certainly shouldn't need it, or am I missing something? Note that Robert withdrew his complaint about relying on indexing via BackendId in CA+TgmoaFjcji=Hu9YhCSEL0Z116TY2zEKZf7Z5=da+BPTeytoA@mail.gmail.com . I also think you need to initialize *xid/*xmin to InvalidTransactionId if the proc hasn't been found because it exited, otherwise we'll report stale values. Greetings, Andres Freund
pgsql-hackers by date: