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

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.

Regards,

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

On Tue, Dec 17, 2013 at 9:58 AM, Christian Kruse
<christian(at)2ndquadrant(dot)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

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

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(at)2ndquadrant(dot)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

On Wed, Jan 29, 2014 at 3:11 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 14 January 2014 08:38, Christian Kruse <christian(at)2ndquadrant(dot)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

On 30 January 2014 17:27, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jan 29, 2014 at 3:11 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On 14 January 2014 08:38, Christian Kruse <christian(at)2ndquadrant(dot)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

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

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(at)2ndquadrant(dot)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

On Fri, Jan 31, 2014 at 8:02 AM, Christian Kruse
<christian(at)2ndquadrant(dot)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

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(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

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(at)mail(dot)gmail(dot)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(at)2ndquadrant(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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

On Wed, Feb 5, 2014 at 1:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Feb 5, 2014 at 1:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On 2014-02-05 13:26:15 -0500, Robert Haas wrote:
> On Wed, Feb 5, 2014 at 1:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)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

Hi,

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

Best regards,

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

On Fri, Feb 7, 2014 at 4:34 AM, Christian Kruse
<christian(at)2ndquadrant(dot)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

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

Hi,

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.

Best regards,

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

On Wed, Feb 12, 2014 at 8:00 AM, Christian Kruse
<christian(at)2ndquadrant(dot)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

On 2014-02-14 23:03:43 -0500, Robert Haas wrote:
> On Wed, Feb 12, 2014 at 8:00 AM, Christian Kruse
> <christian(at)2ndquadrant(dot)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

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

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

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

Hi,

attached you will find a new version of the patch with a second getter
function.

Best regards,

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

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

Hi,

On 21/02/14 13:49, Andres Freund wrote:
> 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?

Makes sense, good idea. Attached you will find a new version of the
patch.

Best regards,

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

Hi,

and another version with fixed comment style… sorry for traffic :)

Best regards,

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

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

Hi,

attached you will find a fixed version.

Best regards,

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

On Mon, Feb 24, 2014 at 2:49 PM, Christian Kruse
<christian(at)2ndquadrant(dot)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

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