Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements

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