Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
Hi Robert, On 25/02/14 12:37, Robert Haas wrote: Committed, after fixing the regression test outputs. I also changed the new fields to be NULL rather than 0 when they are invalid, because that way applying age() to that column does something useful instead of something lame. Hm, thought I had done that. However, thanks for committing and fixing! Best regards, -- Christian Kruse http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services pgpBD5IQLGElj.pgp Description: PGP signature
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On Mon, Feb 24, 2014 at 2:49 PM, Christian Kruse christ...@2ndquadrant.com wrote: attached you will find a fixed version. Committed, after fixing the regression test outputs. I also changed the new fields to be NULL rather than 0 when they are invalid, because that way applying age() to that column does something useful instead of something lame. -- 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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
Hi,
On 2014-02-21 14:15:09 +0100, Christian Kruse wrote:
+/* --
+ * pgstat_fetch_stat_local_beentry() -
+ *
+ * Like pgstat_fetch_stat_beentry() but with local addtions (like xid and
+ * xmin values of the backend)
s/local addtions/locally computed addititions/
+/* --
+ * LocalPgBackendStatus
+ *
+ * When we build the backend status array, we use LocalPgBackendStatus to be
+ * able to add new values to the struct when needed without adding new fields
+ * to the shared memory. It contains the backend status as a first member.
+ * --
+ */
+typedef struct LocalPgBackendStatus
+{
+ /*
+ * Local version of the backend status entry
+ */
+ PgBackendStatus backendStatus;
+
+ /*
+ * The xid of the current transaction if available, InvalidTransactionId
+ * if not
+ */
+ TransactionId backend_xid;
+
+ /*
+ * The xmin of the current session if available, InvalidTransactionId
+ * if not
+ */
+ TransactionId backend_xmin;
+} LocalPgBackendStatus;
+
Those are sentences, so they should include a . at the end.
I think this is ready for committer.
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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On 2014-02-21 13:40:59 +0100, Christian Kruse wrote:
diff --git a/src/backend/utils/adt/pgstatfuncs.c
b/src/backend/utils/adt/pgstatfuncs.c
index a4f31cf..e65b079 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -536,7 +536,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
oldcontext =
MemoryContextSwitchTo(funcctx-multi_call_memory_ctx);
- tupdesc = CreateTemplateTupleDesc(14, false);
+ tupdesc = CreateTemplateTupleDesc(16, false);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, datid,
OIDOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, pid,
@@ -565,6 +565,10 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 14, client_port,
INT4OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 15, backend_xid,
+XIDOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 16, backend_xmin,
+XIDOID, -1, 0);
funcctx-tuple_desc = BlessTupleDesc(tupdesc);
@@ -588,11 +592,11 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
for (i = 1; i = n; i++)
{
- PgBackendStatus *be =
pgstat_fetch_stat_beentry(i);
+ LocalPgBackendStatus *be =
pgstat_fetch_stat_local_beentry(i);
if (be)
{
- if (be-st_procpid == pid)
+ if (be-backendStatus.st_procpid
== pid)
If we're going this route - which I am ok with - I'd suggest for doing
something like:
- PgBackendStatus *be =
pgstat_fetch_stat_beentry(i);
+ LocalPgBackendStatus *lbe =
pgstat_fetch_stat_local_beentry(i);
+ PgBackendStatus *be = lb-backendStatus;
There seems little point in making all those lines longer and the
accompanying diff noise if all it costs is a local variable.
Makes sense?
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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
Hi, On 18.02.2014 22:02, Andres Freund wrote: Not really sure which way is better. One dev against it, one dev not sure. Enough for me to change it :) Will post a new patch this evening. Best regards, -- Christian Kruse http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On 2014-02-17 10:44:41 +0100, Christian Kruse wrote: This is true for now. But one of the purposes of using LocalPgBackendStatus instead of PgBackendStatus was to be able to add more fields like this in future. And thus we might need to change this in future, so why not do it now? I don't think that argument is particularly valid. All (?) the other functions just return just one value, so they won't ever have the need to return more. Not really sure which way is better. 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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
Hi Robert, Am 15.02.14 05:03, schrieb Robert Haas: Well, this version of the patch reveals a mighty interesting point: a lot of the people who are calling pgstat_fetch_stat_beentry() don't need this additional information and might prefer not to pay the cost of fetching it. Well, the cost is already paid due to the fact that this patch uses LocalPgBackendStatus instead of PgBackendStatus in pgstat_read_current_status(). And pgstat_fetch_stat_beentry() returns a pointer instead of a copy, so the cost is rather small, too. None of pg_stat_get_backend_pid, pg_stat_get_backend_dbid, pg_stat_get_backend_userid, pg_stat_get_backend_activity, pg_stat_get_backend_activity, pg_stat_get_backend_waiting, pg_stat_get_backend_activity_start, pg_stat_get_backend_xact_start, pg_stat_get_backend_start, pg_stat_get_backend_client_addr, pg_stat_get_backend_client_port, pg_stat_get_backend_client_port, and pg_stat_get_db_numbackends actually need this new information; it's only ever used in one place. So it seems like it might be wise to have pgstat_fetch_stat_beentry continue to return the PgBackendStatus * and add a new function pgstat_fetch_stat_local_beentry to fetch the LocalPgBackendStatus *; then most of these call sites wouldn't need to change. This is true for now. But one of the purposes of using LocalPgBackendStatus instead of PgBackendStatus was to be able to add more fields like this in future. And thus we might need to change this in future, so why not do it now? And I also agree to Andres. It would still be the case that pgstat_read_current_status() pays the price of fetching this information even if pg_stat_get_activity is never called. But since that's probably by far the most commonly-used API for this information, that's probably OK. I agree. I will change it if this is really wanted, but I think it would be a good idea to do it this way. Best regards, -- Christian Kruse http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On 2014-02-14 23:03:43 -0500, Robert Haas wrote: On Wed, Feb 12, 2014 at 8:00 AM, Christian Kruse christ...@2ndquadrant.com wrote: On Wednesday 12 February 2014 11:14:56 Andres Freund wrote: But they do take up shared memory without really needing to. I personally don't find that too bad, it's not much memory. If we want to avoid it we could have a LocalPgBackendStatus that includes the normal PgBackendStatus. Since pgstat_read_current_status() copies all the data locally, that'd be a sensible point to fill it. While that will cause a bit of churn, I'd guess we can use the infrastructure in the not too far away future for other parts. That's a good idea. Attached you will find a patch implementing it that way; is this how you pictured it? Although I'm not sure if this shouldn't be done in two patches, one for the changes needed for LocalPgBackendStatus and one for the xid/xmin changes. Well, this version of the patch reveals a mighty interesting point: a lot of the people who are calling pgstat_fetch_stat_beentry() don't need this additional information and might prefer not to pay the cost of fetching it. None of [functions] actually need this new information; it's only ever used in one place. So it seems like it might be wise to have pgstat_fetch_stat_beentry continue to return the PgBackendStatus * and add a new function pgstat_fetch_stat_local_beentry to fetch the LocalPgBackendStatus *; then most of these call sites wouldn't need to change. Hm maybe, seems to be as advantageous as not. Less lines changed, but a essentially duplicate function present. It would still be the case that pgstat_read_current_status() pays the price of fetching this information even if pg_stat_get_activity is never called. But since that's probably by far the most commonly-used API for this information, that's probably OK. Agreed. 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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On Wed, Feb 12, 2014 at 8:00 AM, Christian Kruse christ...@2ndquadrant.com wrote: On Wednesday 12 February 2014 11:14:56 Andres Freund wrote: But they do take up shared memory without really needing to. I personally don't find that too bad, it's not much memory. If we want to avoid it we could have a LocalPgBackendStatus that includes the normal PgBackendStatus. Since pgstat_read_current_status() copies all the data locally, that'd be a sensible point to fill it. While that will cause a bit of churn, I'd guess we can use the infrastructure in the not too far away future for other parts. That's a good idea. Attached you will find a patch implementing it that way; is this how you pictured it? Although I'm not sure if this shouldn't be done in two patches, one for the changes needed for LocalPgBackendStatus and one for the xid/xmin changes. Well, this version of the patch reveals a mighty interesting point: a lot of the people who are calling pgstat_fetch_stat_beentry() don't need this additional information and might prefer not to pay the cost of fetching it. None of pg_stat_get_backend_pid, pg_stat_get_backend_dbid, pg_stat_get_backend_userid, pg_stat_get_backend_activity, pg_stat_get_backend_activity, pg_stat_get_backend_waiting, pg_stat_get_backend_activity_start, pg_stat_get_backend_xact_start, pg_stat_get_backend_start, pg_stat_get_backend_client_addr, pg_stat_get_backend_client_port, pg_stat_get_backend_client_port, and pg_stat_get_db_numbackends actually need this new information; it's only ever used in one place. So it seems like it might be wise to have pgstat_fetch_stat_beentry continue to return the PgBackendStatus * and add a new function pgstat_fetch_stat_local_beentry to fetch the LocalPgBackendStatus *; then most of these call sites wouldn't need to change. It would still be the case that pgstat_read_current_status() pays the price of fetching this information even if pg_stat_get_activity is never called. But since that's probably by far the most commonly-used API for this information, that's probably OK. -- 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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On 2014-02-11 09:15:45 -0500, Robert Haas wrote: 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. I don't think the will be unitialized, pgstat_fetch_stat_beentry() will do a pgstat_read_current_status() if neccessary which will initialize them. But they do take up shared memory without really needing to. I personally don't find that too bad, it's not much memory. If we want to avoid it we could have a LocalPgBackendStatus that includes the normal PgBackendStatus. Since pgstat_read_current_status() copies all the data locally, that'd be a sensible point to fill it. While that will cause a bit of churn, I'd guess we can use the infrastructure in the not too far away future for other parts. 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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On Fri, Feb 7, 2014 at 4:34 AM, Christian Kruse christ...@2ndquadrant.com wrote: attached you will find a new version with the following issues resolved: - use backend ID once again for getting the xid and xmin - use xref instead of link in documentation - rename fields to backend_xid and backend_xmin This looks mostly good but it doesn't address this point, from one of my earlier emails: 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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On 2014-02-05 13:26:15 -0500, Robert Haas wrote: On Wed, Feb 5, 2014 at 1:21 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@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. Those work for me. We have a bit of a confusing situation atm, pg_prepared_xact calls it's xid transaction, pg_locks transactionid... So if we add a new speling, we should like it sufficiently to use it in the future. 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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On Mon, Feb 3, 2014 at 6:47 AM, Christian Kruse christ...@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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
Robert Haas robertmh...@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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On Wed, Feb 5, 2014 at 1:21 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
Robert Haas robertmh...@gmail.com writes: On Wed, Feb 5, 2014 at 1:21 PM, Tom Lane t...@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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
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: + entryThe current xref linked=ddl-system-columnsxmin 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 pgpkstlLLohOH.pgp Description: PGP signature
Re: [HACKERS] 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: replaceableuser/ replaceabledatabase/
replaceablehost/ re
/entry
/row
row
+ entrystructfieldtransactionid/structfield/entry
+ entrytypexid/type/entry
+ entryThe current transaction identifier./entry
+/row
The other entries refer to the current backend. Maybe
Toplevel transaction identifier of this backend, if any.
+row
+ entrystructfieldxmin/structfield/entry
+ entrytypexid/type/entry
+ entryThe current xref linked=ddl-system-columnsxmin/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.
entrystructfieldquery//entry
entrytypetext//entry
entryText of this backend's most recent query. If
@@ -1484,6 +1494,11 @@ postgres: replaceableuser/
replaceabledatabase/ replaceablehost/ re
/entry
/row
row
+ entrystructfieldxmin/structfield/entry
+ entrytypexid/type/entry
+ entryThe current xref linked=ddl-system-columnsxmin
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+bptey...@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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
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: + entryThe current xref linked=ddl-system-columnsxmin value./xref/entry Can you elaborate? Best regards, -- Christian Kruse http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services pgp70zYmW2633.pgp Description: PGP signature
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On Fri, Jan 31, 2014 at 4:40 AM, Andres Freund and...@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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On Fri, Jan 31, 2014 at 8:02 AM, Christian Kruse christ...@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: + entryThe current xref linked=ddl-system-columnsxmin 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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On Wed, Jan 29, 2014 at 3:11 PM, Simon Riggs si...@2ndquadrant.com wrote: On 14 January 2014 08:38, Christian Kruse christ...@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: + entryThe current xref linked=ddl-system-columnsxmin 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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On 30 January 2014 17:27, Robert Haas robertmh...@gmail.com wrote: On Wed, Jan 29, 2014 at 3:11 PM, Simon Riggs si...@2ndquadrant.com wrote: On 14 January 2014 08:38, Christian Kruse christ...@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: + entryThe current xref linked=ddl-system-columnsxmin 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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On 14 January 2014 08:38, Christian Kruse christ...@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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
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
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
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 pgp5adohHq0q6.pgp Description: PGP signature
Re: [HACKERS] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On Tue, Dec 17, 2013 at 9:58 AM, Christian Kruse christ...@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
