Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements

From: Andrei Zubkov <zubkov(at)moonset(dot)ru>
To: Andres Freund <andres(at)anarazel(dot)de>, Julien Rouhaud <rjuju123(at)gmail(dot)com>
Cc: Greg Stark <stark(at)mit(dot)edu>, "Anton A(dot) Melnikov" <aamelnikov(at)inbox(dot)ru>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements
Date: 2022-04-02 10:12:54
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On Fri, 2022-04-01 at 13:01 -0700, Andres Freund wrote:
> It seems decidedly not great to have four copies of this code. It was
> already
> not great before, but this patch makes the duplicated section go from
> four
> lines to 20 or so.

Agreed. I've created the single_entry_reset() function wrapping this
code. I wonder if it should be declared as inline to speedup a little.

On Sat, 2022-04-02 at 15:10 +0800, Julien Rouhaud wrote:
> > However
> > we can test at least functionality of stats_reset field like this:
> >
> > SELECT now() AS ref_ts \gset
> > SELECT dealloc, stats_reset >= :'ref_ts' FROM
> > pg_stat_statements_info;
> > SELECT pg_stat_statements_reset();
> > SELECT dealloc, stats_reset >= :'ref_ts' FROM
> > pg_stat_statements_info;
> >
> > Does it seems reasonable?
> It looks reasonable, especially if the patch adds a new mode for the
> reset
> function.

I've implemented this test.

> > Checking
> > of only execution stats seems enough to me - in most cases we can't
> > check planning stats with such test anyway.
> > What do you think about it?
> Ah I see. I guess we could set plan_cache_mode to force_generic_plan
> to make
> sure we go though planning. But otherwise just adding a comment
> saying that
> the test has to be compatible with different plan caching approach
> would be
> fine with me.

Set plan_cache_mode seems a little bit excess to me. And maybe in the
future some another plan caching strategies will be implementd with
coresponding settings.. So I've just left a comment there.

On Sat, 2022-04-02 at 15:21 +0800, Julien Rouhaud wrote:
> I'm not sure about returning the ts. If you need it you could call
> now() FROM pg_stat_statements_reset() (or clock_timestamp()). It
> won't be
> entirely accurate but since the function will have an exclusive lock
> during the
> whole execution that shouldn't be a problem. Now you're already
> adding a new
> version of the C function so I guess that it wouldn't require any
> additional
> effort so why not.

I think that if we can do it in accurate way and there is no obvious
side effects, why not to try it...
Changing of pg_stat_statements_reset function result caused a
confiderable tests update. Also, I'm not sure that my description of
this feature in the docs is blameless..

After all, I'm a little bit in doubt about this feature, so I'm ready
to rollback it.

v9 attached
regards, Andrei

Attachment Content-Type Size
v9-0001-pg_stat_statements-Track-statement-entry-timestamp.patch text/x-patch 63.2 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2022-04-02 10:26:31 Re: A qsort template
Previous Message Amit Kapila 2022-04-02 10:04:01 Re: Skipping logical replication transactions on subscriber side