Re: Re[2]: [PATCH] Add last_executed timestamp to pg_stat_statements - Mailing list pgsql-hackers

From Sami Imseih
Subject Re: Re[2]: [PATCH] Add last_executed timestamp to pg_stat_statements
Date
Msg-id CAA5RZ0u6qdcLoWJJz5mUB_VYZqT__U-HJJ5_ioYO7XGsUfXfpw@mail.gmail.com
Whole thread Raw
In response to Re[2]: [PATCH] Add last_executed timestamp to pg_stat_statements  ("Pavlo Golub" <pavlo.golub@cybertec.at>)
List pgsql-hackers
> >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)



pgsql-hackers by date:

Previous
From: Jim Mlodgenski
Date:
Subject: Re: INOUT params with expanded objects
Next
From: Tom Lane
Date:
Subject: Re: Fix and improve allocation formulas