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

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