MIME-Version: 1.0
References: 
 <CAK7ymc+FxoVswo1ok_xDW-xPG-ZEZ8SAqCUkJ7WF04=0aQDvVQ@mail.gmail.com>
 <CAA5RZ0uyvnNuHRj=73jZhocnLxE07oM-AeSLS=FUuuZ_hy7fKA@mail.gmail.com>
 <em5be966c3-9086-4f3f-b516-97444b854fdf@10e48d49.com>
In-Reply-To: <em5be966c3-9086-4f3f-b516-97444b854fdf@10e48d49.com>
From: Sami Imseih <samimseih@gmail.com>
Date: Thu, 11 Dec 2025 10:35:41 -0600
Message-ID: 
 <CAA5RZ0u6qdcLoWJJz5mUB_VYZqT__U-HJJ5_ioYO7XGsUfXfpw@mail.gmail.com>
Subject: Re: Re[2]: [PATCH] Add last_executed timestamp to pg_stat_statements
To: Pavlo Golub <pavlo.golub@cybertec.at>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA5RZ0u6qdcLoWJJz5mUB_VYZqT__U-HJJ5_ioYO7XGsUfXfpw%40mail.gmail.com>
Precedence: bulk

> >Can pg_stat_statements.stats_since help here?
> >
> >for example "where stats_since > last_poll_timestamp" ?
>
> Actually no, monitoring tools fetch snapshots to find the difference
> between snapshots.
> Data for every statement is changes after each execution.
>
> But stats_since is inserted only once when the new statement execution
> appears and is never updated during next executions.

I was thinking of using stats_since to avoid fetching query text,
since that does not change. But you are talking about avoiding all
the stats if they have not changed. I see that now.

FWIW, this was discussed back in 2017 [0], and at that time there was
some support for last_executed, but the patch did not go anywhere.

After looking at the patch, I have a few comments:

1/ There are whitespace errors when applying.

2/ Calling GetCurrentTimestamp while holding a spinlock is
not a good idea and should be avoided. This was also a point
raised in [0]. Even when we move pg_stat_statements
to cumulative stats and not at the mercy of the spinlock for updating
entries, i would still hesitate to add an additional GetCurrentTimestamp()
for every call.

I wonder if we can use GetCurrentStatementStartTimestamp()
instead?

```
/*
* GetCurrentStatementStartTimestamp
*/
TimestampTz
GetCurrentStatementStartTimestamp(void)
{
return stmtStartTimestamp;
}
```

stmtStartTimestamp is the time the query started, which seems OK for
the use-case you are mentioning. But also, stmtStartTimestamp gets
set at the top-level so nested entries (toplevel = false ) will just
inherit the timestamp of the top-level entry.

IMO, this is the most important point in the patch for now.

3/ last_executed, or maybe (last_toplevel_start) if we go with #2 should not
be added under pgssEntry->Counters, but rather directory under pgssEntry.

@@ -213,6 +214,7 @@ typedef struct Counters
                                              * launched */
     int64        generic_plan_calls; /* number of calls using a generic plan */
     int64        custom_plan_calls;    /* number of calls using a
custom plan */
+    TimestampTz last_executed;    /* timestamp of last statement execution */
 } Counters;

4/ instead of a " last_executed" maybe the tests should be added to
entry_timestamp.sql?


[0] https://www.postgresql.org/message-id/flat/CA%2BTgmoZgZMeuN8t9pawSt6M%3DmvxKiAZ4CvPofBWwwVWeZwHe4w%40mail.gmail.com#beeebe3ca4a3dcda4ed625f7c15bb2d8

--
Sami Imseih
Amazon Web Services (AWS)