MIME-Version: 1.0
References: <em5be966c3-9086-4f3f-b516-97444b854fdf@10e48d49.com>
 <CAA5RZ0u6qdcLoWJJz5mUB_VYZqT__U-HJJ5_ioYO7XGsUfXfpw@mail.gmail.com>
 <aYIh7I9khvoewwH_@msg.df7cb.de> <CAA5RZ0tH_+3JGMCktkp8EMyvKaRAJf-xRh+y_UoZ8azvuUEAYQ@mail.gmail.com>
 <aYMKolTlvzenr4Pk@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAK7ymc+E00zKC9Da+BK2se=O2RiOUkdW5dz4iAynD1Vj=Pwexg@mail.gmail.com>
 <CAA5RZ0u_jjiLc_w4nksdVazvvqtE17C1nM=sKf9fy5x02orfsg@mail.gmail.com>
 <emed32d635-e46b-4f08-9085-3fd822fe5765@cybertec.at> <aYXH4hkFc_2xOa8t@msg.df7cb.de>
 <ema0100e49-522e-4b8f-9c25-9257af1f0793@cybertec.at> <aYXcteoB-JSqP4m9@msg.df7cb.de>
In-Reply-To: <aYXcteoB-JSqP4m9@msg.df7cb.de>
From: Sami Imseih <samimseih@gmail.com>
Date: Fri, 6 Feb 2026 09:47:07 -0600
Message-ID: <CAA5RZ0vMW+0_6FYtFEaN-NAnk1sT2ucCUDJe0-xKwZriROV--A@mail.gmail.com>
Subject: Re: [PATCH] Add last_executed timestamp to pg_stat_statements
To: Christoph Berg <myon@debian.org>
Cc: Pavlo Golub <pavlo.golub@cybertec.at>, 
	Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, 
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAA5RZ0vMW%2B0_6FYtFEaN-NAnk1sT2ucCUDJe0-xKwZriROV--A%40mail.gmail.com>
Precedence: bulk

> >   WHERE last_execution_start + max_exec_time * INTERVAL '1 ms' > NOW() -
> > polling_interval
>
> Is this extra complexity worth one saved GetCurrentTimestamp()?
>
> src/backend/access/transam/xact.c is calling GetCurrentTimestamp a
> lot already, so I don't really buy the argument it should be avoided
> at all cost in pg_stat_statements. Just storing the statement end time
> would make this use case much nicer.

We do call GetCurrentTimestamp() to set timestamps for xact_start, etc.

But, we also take special care to avoid extra calls.

```
/*
* set transaction_timestamp() (a/k/a now()). Normally, we want this to
* be the same as the first command's statement_timestamp(), so don't do a
* fresh GetCurrentTimestamp() call (which'd be expensive anyway). But
* for transactions started inside procedures (i.e., nonatomic SPI
* contexts), we do need to advance the timestamp. Also, in a parallel
* worker, the timestamp should already have been provided by a call to
* SetParallelStartTimestamps().
*/
if (!IsParallelWorker())
{
if (!SPI_inside_nonatomic_context())
xactStartTimestamp = stmtStartTimestamp;
else
xactStartTimestamp = GetCurrentTimestamp();
```


> OK, here is one more try. I discovered the `total_time` argument to
> the `pgss_store()` function! So we can calculate the finish time
> without calling `GetCurrentTimestamp()`.
>
> This is version 3 of the patch adding a `stats_last_updated` column
> (yes, again) to pg_stat_statements. Based on feedback, this version
> improves the implementation with better performance and correctness.
>
> The main improvement uses `statement_start + execution_duration` with
> `rint(total_time * 1000.0)` to convert milliseconds to microseconds
> with proper rounding. The calculation performed BEFORE acquiring
> spinlock and assigned within locked scope.

Correct, this also crossed my mind. Although I would consider doing things a bit
different. Instead of calculating the end time in a single column, I still
think it's worth having a last_executed_start and a
last_execution_time (duration),
and the user can calculate this on the SQL layer. I think it's better because
last_execution_start is already a known timestamp in
pg_stat_activity.query_start
and some tool that finds a long running query in pg_stat_activity, knowing the
query_start they could then go look it up in pg_stat_statements. What I'm
really getting at is separating these fields will open up more use cases, IMO.

Of course, we are adding 2 new columns instead of just 1, but these values
do have benefits.

--
Sami Imseih
Amazon Web Services (AWS)