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.
