From: | Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> |
---|---|
To: | Andrei Zubkov <zubkov(at)moonset(dot)ru>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Cc: | Greg Stark <stark(at)mit(dot)edu>, Andres Freund <andres(at)anarazel(dot)de>, Julien Rouhaud <rjuju123(at)gmail(dot)com> |
Subject: | Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements |
Date: | 2023-01-18 16:29:43 |
Message-ID: | 34168f7a-af01-9e6a-4c82-6b3a2864e4f5@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
I took a quick look at this patch, to see if there's something we
want/can get into v16. The last version was submitted about 9 months
ago, and it doesn't apply cleanly anymore, but the bitrot is fairly
minor. Not sure there's still interest, though.
As for the patch, I wonder if it's unnecessarily complex. It adds *two*
timestamps for each pg_stat_statements entry - one for reset of the
whole entry, one for reset of "min/max" times only.
I can see why the first timestamp (essentially tracking creating of the
entry) is useful. I'd probably call it "created_at" or something like
that, but that's a minor detail. Or maybe stats_reset, which is what we
use in pgstat?
But is the second timestamp for the min/max fields really useful? AFAIK
to perform analysis, people take regular pg_stat_statements snapshots,
which works fine for counters (calculating deltas) but not for gauges
(which need a reset, to track fresh values). But people analyzing this
are already resetting the whole entry, and so the snapshots already are
tracking deltas.
So I'm not convinced actually need the second timestamp.
A couple more comments:
1) I'm not sure why the patch is adding tests of permissions on the
pg_stat_statements_reset function?
2) If we want the second timestamp, shouldn't it also cover resets of
the mean values, not just min/max?
3) I don't understand why the patch is adding "IS NOT NULL AS t" to
various places in the regression tests.
4) I rather dislike the "minmax" naming, because that's often used in
other contexts (for BRIN indexes), and as I mentioned maybe it should
also cover the "mean" fields.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From | Date | Subject | |
---|---|---|---|
Next Message | Bharath Rupireddy | 2023-01-18 16:34:31 | Re: Improve GetConfigOptionValues function |
Previous Message | Robert Haas | 2023-01-18 16:28:57 | Re: almost-super-user problems that we haven't fixed yet |