From: | Andres Freund <andres(at)2ndquadrant(dot)com> |
---|---|
To: | Christian Kruse <christian(at)2ndquadrant(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Patch: show xid and xmin in pg_stat_activity and pg_stat_replication |
Date: | 2014-02-01 16:23:06 |
Message-ID: | 20140201162306.GE32407@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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(at)mail(dot)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Fujii Masao | 2014-02-01 16:46:54 | Re: [PATCH] Support for pg_stat_archiver view |
Previous Message | Christian Kruse | 2014-02-01 16:03:46 | Re: Patch: show xid and xmin in pg_stat_activity and pg_stat_replication |