Patch: show xid and xmin in pg_stat_activity and pg_stat_replication

On Tue, Dec 17, 2013 at 9:58 AM, Christian Kruse
<christian@2ndquadrant.com> wrote:

Hi,

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.

This may be helpful when looking for the cause of bloat.

I added two new struct members in PgBackendStatus which get filled in
pgstat_read_current_status() and slightly modified the catalog schema
and the pg_stat_get_activity() procedure.

I'm not sure if it is a good idea to gather the data in
pgstat_read_current_status(), but I chose to do it this way
nonetheless because otherwise I would have to create collector
functions like pgstat_report_xid_assignment() /
pgstat_report_xmin_changed() (accordingly to
pgstat_report_xact_timestamp()) which may result in a performance hit.

Please add your patch here so we don't lose track of it:

https://commitfest.postgresql.org/action/commitfest_view/open

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 17/12/13 12:08, Robert Haas wrote:

Please add your patch here so we don't lose track of it:

https://commitfest.postgresql.org/action/commitfest_view/open

Thanks. I nearly forgot that.

Regards,

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

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.

When an admin is looking for a long-running transaction that's blocking
vacuum, he will currently rely on the timestamp fields, xact_start and
query_start. I'm not sure how much extra value this adds over those
timestamps in pg_stat_activity, but there are not such fields in
pg_stat_replication, so that part is definitely useful. And if we're
going to add xmin to pg_stat_replication, it makes sense to add it to
pg_stat_activity too. Unless someone can come up with something better
to display for walsenders. The timestamp of the last commit record
that's been replayed, perhaps?

What else would a user would want to do with these?

This definitely sounds useful to me as a developer, though. So I'm
thinking we should add these for that reason, in any case.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

Regards,

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

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?

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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>

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-01-30 12:27:43 -0500, Robert Haas wrote:

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.

Hm. I don't see how that's going to be broken without major surgery in
pgstat.c. The whole thing seems to rely on being able to index
BackendStatusArray with MyBackendId?

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:

It certainly needs to be documented as racy, but I don't see a big
problem with being racy here. We assume in lots of places that
writing/reading xids is atomic, and we don't even hold exclusive locks
while writing... (And yes, that means that the xid and xmin don't
necessarily belong to each other)
That said, encapsulating that racy access into a accessor function does
sound like a good plan.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

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.

what do you think about the approach the attached patch implements?
I'm not really sure if this is what you had in mind, especially if
this is the right 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>

Can you elaborate?

Best regards,

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

On Fri, Jan 31, 2014 at 4:40 AM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2014-01-30 12:27:43 -0500, Robert Haas wrote:

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.

Hm. I don't see how that's going to be broken without major surgery in
pgstat.c. The whole thing seems to rely on being able to index
BackendStatusArray with MyBackendId?

Oh, you're right. pgstat_initialize() sets it up that way.

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:

It certainly needs to be documented as racy, but I don't see a big
problem with being racy here. We assume in lots of places that
writing/reading xids is atomic, and we don't even hold exclusive locks
while writing... (And yes, that means that the xid and xmin don't
necessarily belong to each other)
That said, encapsulating that racy access into a accessor function does
sound like a good plan.

Yep, shouldn't be hard to do.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jan 31, 2014 at 8:02 AM, Christian Kruse
<christian@2ndquadrant.com> wrote:

what do you think about the approach the attached patch implements?
I'm not really sure if this is what you had in mind, especially if
this is the right lock.

The attached patch seems not to be attached, but the right lock to use
would be the same one BackendIdGetProc() is using. I'd add a new
function BackendIdGetTransactionIds or something like that.

I also note that the docs seem to need some copy-editing:

+ <entry>The current <xref linked="ddl-system-columns">xmin
value.</xref></entry>

The link shouldn't include the period, and probably should also not
include the word "value". I would make only the word "xmin" part of
the link.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 31/01/14 11:24, Robert Haas wrote:

what do you think about the approach the attached patch implements?
I'm not really sure if this is what you had in mind, especially if
this is the right lock.

The attached patch seems not to be attached, […]

*sighs*
I'm at FOSDEM right now, I will send it as soon as I'm back home.

[…] but the right lock to use would be the same one
BackendIdGetProc() is using. I'd add a new function
BackendIdGetTransactionIds or something like that.

Good – that's exactly what I did (with a slightly different naming).

I also note that the docs seem to need some copy-editing:

+ <entry>The current <xref linked="ddl-system-columns">xmin
value.</xref></entry>

The link shouldn't include the period, and probably should also not
include the word "value". I would make only the word "xmin" part of
the link.

Thanks for elaboration.

Best regards,

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

Hi,

managed to get some time to prepare the patch at FOSDEM. Attached
(this time for real ;-) you will find a new patch version.

Best regards,

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Andres,

ok, lesson learned: don't refactor patches in a rush ;)

[… documation issues …]

Should be fixed according to your critics.

@@ -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?

Right. Removed.
(This file creates so many warnings with -W -Wall -ansi -pedantic that
I didn't notice that specific warning)

/*
+ * 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.

Originally I returned PGPROC to kill to birds with one stone and to be
able to distinguish an error case, e.g. when the backend could not be
found. But in favor of code quality I think your complaint is right
and I should not do that. Instead now xid and xmin are initialized
with InvalidTransactionId.

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 .

Because I was in a rush. Originally I wrote this getter in reaction to
Robert's complaints and refactored it when he took back. But I forgot
to remove the loop, too. Won't happen again, I won't refactor in a
hurry again ;-)

Attached you will find an updated version of the patch.

Best regards,

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

On Mon, Feb 3, 2014 at 6:47 AM, Christian Kruse
<christian@2ndquadrant.com> wrote:

[ new patch ]

Is there some compelling reason not to write the documentation link as
<xref linkend="guc-hot-standby-feedback"> rather than using <link>?

It feels weird to me that the new columns are called transactionid and
xmin. Why not xid and xmin?

If I understand correctly, modifying PgBackendStatus adds additional
fields to the shared memory data structure that are never used and
will be returned by functions like pgstat_fetch_stat_beentry()
unitialized. That seems both inefficient and a pitfall for the
unwary.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

It feels weird to me that the new columns are called transactionid and
xmin. Why not xid and xmin?

Actually the part of that that bothers me is "xmin", which conflicts
with a reserved system column name. While you can legally pick such
conflicting names for view columns, it's not going to be so much fun
when you try to join that view against some regular table.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Feb 5, 2014 at 1:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

It feels weird to me that the new columns are called transactionid and
xmin. Why not xid and xmin?

Actually the part of that that bothers me is "xmin", which conflicts
with a reserved system column name. While you can legally pick such
conflicting names for view columns, it's not going to be so much fun
when you try to join that view against some regular table.

That's a fair point, too. So maybe we should go with something like
backend_xid and backend_xmin or some other prefix that we come up
with. My concern is more that I think they should be consistent
somehow.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Feb 5, 2014 at 1:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Actually the part of that that bothers me is "xmin", which conflicts
with a reserved system column name. While you can legally pick such
conflicting names for view columns, it's not going to be so much fun
when you try to join that view against some regular table.

That's a fair point, too. So maybe we should go with something like
backend_xid and backend_xmin or some other prefix that we come up
with. My concern is more that I think they should be consistent
somehow.

Works for me.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers