On Wed, Jan 20, 2021 at 6:15 PM Julien Rouhaud <rjuju...@gmail.com> wrote: > > Hi, > > On Tue, Jan 19, 2021 at 4:55 PM Masahiro Ikeda <ikeda...@oss.nttdata.com> > wrote: > > > > Thanks for making the patch to add "toplevel" column in > > pg_stat_statements. > > This is a review comment. > > Thanks a lot for the thorough review! > > > I tested the "update" command can work. > > postgres=# ALTER EXTENSION pg_stat_statements UPDATE TO '1.10'; > > > > Although the "toplevel" column of all queries which already stored is > > 'false', > > we have to decide the default. I think 'false' is ok. > > Mmm, I'm not sure that I understand this result. The "toplevel" value > is tracked by the C code loaded at startup, so it should have a > correct value even if you used to have the extension in a previous > version, it's just that you can't access the toplevel field before > doing the ALTER EXTENSION pg_stat_statements UPDATE. There's also a > change in the magic number, so you can't use the stored stat file from > a version before this patch. > > I also fail to reproduce this behavior. I did the following: > > - start postgres with pg_stat_statements v1.10 (so with this patch) in > shared_preload_libraries > - CREATE EXTENSION pg_stat_statements VERSION '1.9'; > - execute a few queries > - ALTER EXTENSION pg_stat_statements UPDATE; > - SELECT * FROM pg_stat_statements reports the query with toplvel to TRUE > > Can you share a way to reproduce your findings? > > > 2. usability review > > ==================== > > [...] > > Although one concern is whether only top-level is enough or not, > > I couldn't come up with any use-case to use nested level, so I think > > it's ok. > > I agree, I don't see how tracking statistics per nesting level would > really help. The only additional use case I see would tracking > triggers/FK query execution, but the nesting level won't help with > that. > > > 3. coding review > > ================= > > [...] > > B. add a argument of the pg_stat_statements_reset(). > > > > Now, pg_stat_statements supports a flexible reset feature. > > > pg_stat_statements_reset(userid Oid, dbid Oid, queryid bigint) > > > > Although I wondered whether we need to add a top-level flag to the > > arguments, > > I couldn't come up with any use-case to reset only top-level queries or > > not top-level queries. > > Ah, I didn't think of the reset function. I also fail to see a > reasonable use case to provide a switch for the reset function. > > > 4. others > > ========== > > > > These comments are not related to this patch. > > > > A. about the topic of commitfests > > > > Since I think this feature is for monitoring, > > it's better to change the topic from "System Administration" > > to "Monitoring & Control". > > I agree, thanks for the change!
I've changed the topic accordingly. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/