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

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

In response to

Responses

Browse pgsql-hackers by date

  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