Re: [PATCH v4] pg_stat_statements: Add last_execution_start column

On 13.05.26 16:38, Pavlo Golub wrote:
> Rebased on current master (a3e6beba60e).
> 
> The only conflict was with commit 66366217822 (pg_stat_statements: Set
> PlannedStmt to NULL after nested utility execution), which introduced a
> local copy of pstmt->planOrigin and then nulled pstmt.  The patch context
> line was updated to reference saved_planOrigin instead.  No functional
> changes from v3.
> 
> All pg_stat_statements regression tests pass (16/16).
> 
> v4 patch attached.

Here is a summary of the feedback comments we discussed at pgconf.dev:

(Some of these might have applied to previous patch versions.)

technical:

- obvious concerns: getting current time is expensive (inside spinlock!)
- documenting intended use of the field is good, I would put that back
- issues with long-running queries, start time vs. end time -- ignores 
analytics use cases
- argument total_time of pgss_store() is not documented, should be fixed 
first -- also why is this double??
- issues with extended query protocol can be surprising
- comment says it can be executed under spinlock but then it's not
- only top-level statements? -- not acceptable IMO

code style:

- INT64CONST is useless
- when changing "two" to "three" in a comment, just delete the number

meta/process:

- good: patch versioned, commit message up to date
- I confused me that a new thread was started in the middle of the 
discussion.
- As a theoretical committer, I would like to get Sami's and Christoph's 
feedback on the latest patch version.