On Tue, 2021-08-17 at 02:14 -0700, Andres Freund wrote:
> On 2021-08-17 10:44:51 +0200, Laurenz Albe wrote:
> > On Sun, 2021-08-01 at 13:55 -0700, Andres Freund wrote:
> > > We maintain last_report as GetCurrentTransactionStopTimestamp(), but then
> > > use
> > > a separate timestamp in pgstat_send_connstats() to compute the difference
> > > from
> > > last_report, which is then set to GetCurrentTransactionStopTimestamp()'s
> > > return value.
> >
> > Makes sense to me. How about passing "now", which was just initialized from
> > GetCurrentTransactionStopTimestamp(), as additional parameter to
> > pgstat_send_connstats() and use that value instead of taking the current
> > time?
>
> Yes.
Here is a patch for that.
> > > I'm also not all that happy with sending yet another UDP packet for this.
> >
> > Are you suggesting that connection statistics should be shoehorned into
> > some other statistics message? That would reduce the number of UDP packets,
> > but it sounds ugly and confusing to me.
>
> That ship already has sailed. Look at struct PgStat_MsgTabstat
>
> Given that we transport number of commits/commits, block read/write time
> adding the time the connection was active/inactive doesn't really seem like it
> makes things meaningfully worse?
Point taken.
I looked at the other statistics sent in pgstat_report_stat(), and I see
none that are sent unconditionally. Are you thinking of this:
/*
* Send partial messages. Make sure that any pending xact commit/abort
* gets counted, even if there are no table stats to send.
*/
if (regular_msg.m_nentries > 0 ||
pgStatXactCommit > 0 || pgStatXactRollback > 0)
pgstat_send_tabstat(®ular_msg);
if (shared_msg.m_nentries > 0)
pgstat_send_tabstat(&shared_msg);
I can't think of a way to hack this up that wouldn't make my stomach turn.
> > > Alternatively we could just not send an update to connection stats every
> > > 500ms
> > > for every active connection, but only do so at connection end. The
> > > database
> > > stats would only contain stats for disconnected sessions, while the stats
> > > for
> > > "live" connections are maintained via backend_status.c.
> >
> > That was my original take, but if I remember right, Magnus convinced me that
> > it would be more useful to have statistics for open sessions as well.
> > With a connection pool, connections can stay open for a very long time,
> > and the accuracy and usefulness of the statistics would become questionable.
>
> That's not a contradiction to what I propose. Having the data available via
> backend_status.c allows to sum up the data for active connections and the data
> for past connections.
>
> I think it's also just cleaner to not constantly report changing preliminary
> data as pgstat_send_connstats() does.
Currently, the data are kept in static variables in the backend process.
That would have to change for such an approach, right?
> Doubling the number of UDP messages in common workloads seems also problematic
> enough that it should be addressed for 14.
Ok, but I don't know how to go about it.
Yours,
Laurenz Albe
From 951820a8e312584f283ef4b5bde006c7f41e0d9d Mon Sep 17 00:00:00 2001
From: Laurenz Albe <[email protected]>
Date: Wed, 18 Aug 2021 04:52:57 +0200
Subject: [PATCH] Improve performance and accuracy of session statistics
Rather than calling GetCurrentTimestamp() in pgstat_send_connstats(),
which causes an additional system call, reuse the value just
calculated in pgstat_report_stat().
Since the "now" value in pgstat_report_stat() will become the next
"last_report" value, this will also improve the accuracy of the
statistics, since the timestamp difference calculated in
pgstat_send_connstats() will become the correct one.
This patch does not address the issue that yet another UDP packet
is sent to the statistics collector for session statistics.
Discussion: https://postgr.es/m/20210801205501.nyxzxoelqoo4x2qc%40alap3.anarazel.de
---
src/backend/postmaster/pgstat.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index ce8888cc30..77d734a0ba 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -324,7 +324,7 @@ static void pgstat_send_tabstat(PgStat_MsgTabstat *tsmsg);
static void pgstat_send_funcstats(void);
static void pgstat_send_slru(void);
static HTAB *pgstat_collect_oids(Oid catalogid, AttrNumber anum_oid);
-static void pgstat_send_connstats(bool disconnect, TimestampTz last_report);
+static void pgstat_send_connstats(bool disconnect, TimestampTz last_report, TimestampTz now);
static PgStat_TableStatus *get_tabstat_entry(Oid rel_id, bool isshared);
@@ -879,7 +879,7 @@ pgstat_report_stat(bool disconnect)
/* for backends, send connection statistics */
if (MyBackendType == B_BACKEND)
- pgstat_send_connstats(disconnect, last_report);
+ pgstat_send_connstats(disconnect, last_report, now);
last_report = now;
@@ -1375,7 +1375,7 @@ pgstat_drop_relation(Oid relid)
* ----------
*/
static void
-pgstat_send_connstats(bool disconnect, TimestampTz last_report)
+pgstat_send_connstats(bool disconnect, TimestampTz last_report, TimestampTz now)
{
PgStat_MsgConn msg;
long secs;
@@ -1389,7 +1389,7 @@ pgstat_send_connstats(bool disconnect, TimestampTz last_report)
/* session time since the last report */
TimestampDifference(((last_report == 0) ? MyStartTimestamp : last_report),
- GetCurrentTimestamp(),
+ now,
&secs, &usecs);
msg.m_session_time = secs * 1000000 + usecs;
--
2.26.3
