Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?
Julien Rouhaud <rjuju123@gmail.com>
From: Julien Rouhaud <rjuju123@gmail.com>
To: Bruce Momjian <bruce@momjian.us>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Nitin Jadhav <nitinjadhavpostgres@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Hannu Krosing <hannuk@google.com>, Robert Haas <robertmhaas@gmail.com>, Pavel Stehule <pavel.stehule@gmail.com>, Michael Paquier <michael@paquier.xyz>, torikoshia <torikoshia@oss.nttdata.com>, Atsushi Torikoshi <atorik@gmail.com>, Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>, Tomas Vondra <tomas.vondra@2ndquadrant.com>, Evgeny Efimkin <efimkin@yandex-team.ru>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2021-04-08T00:47:48Z
Lists: pgsql-hackers
On Wed, Apr 07, 2021 at 07:38:35PM -0400, Bruce Momjian wrote: > On Wed, Apr 7, 2021 at 07:01:25PM -0400, Tom Lane wrote: > > Bruce Momjian <bruce@momjian.us> writes: > > > Uh, I think your patch missed a few things. First, you use "%zd" > > > (size_t) for the printf string, but calls to pgstat_get_my_queryid() in > > > src/backend/utils/error/elog.c used "%ld". Which is correct? I see > > > pgstat_get_my_queryid() as returning uint64, but I didn't think a uint64 > > > fits in a BIGINT SQL column. > > > > Neither is correct. Project standard these days for printing [u]int64 > > is to write "%lld" or "%llu", with an explicit (long long) cast on > > the printf argument. > > Yep, got it. The attached patch fixes all the calls to use %lld, and > adds casts. In implementing cvslog, I noticed that internally we pass > the hash as uint64, but output as int64, which I think is a requirement > for how pg_stat_statements has output it, and the use of bigint. Is > that OK? Indeed, this is due to how we expose the value in SQL. The original discussion is at https://www.postgresql.org/message-id/CAH2-WzkueMfAmY3onoXLi+g67SJoKY65Cg9Z1QOhSyhCEU8w3g@mail.gmail.com. As far as I know this is OK, as we want to show consistent values everywhere. > I am also confused about the inconsistency of calling the GUC > compute_query_id (with underscore), but pg_stat_activity.queryid. If we > make it pg_stat_activity.query_id, it doesn't match most of the other > *id columsns in the table, leader_pid, usesysid, backend_xid. Is that > OK?I know I suggested pg_stat_activity.query_id, but maybe I was wrong. Mmm, most of the columns in pg_stat_activity do have a "_", so using query_id would make more sense. @@ -2967,6 +2967,10 @@ write_csvlog(ErrorData *edata) appendStringInfoChar(&buf, '\n'); + /* query id */ + appendStringInfo(&buf, "%lld", (long long) pgstat_get_my_queryid()); + appendStringInfoChar(&buf, ','); + /* If in the syslogger process, try to write messages direct to file */ if (MyBackendType == B_LOGGER) write_syslogger_file(buf.data, buf.len, LOG_DESTINATION_CSVLOG); Unless I'm missing something this will output the query id in the next log line? The new code should be added before the newline is output, and the comma should also be output before the queryid.
Commits
-
Clarify description of pg_stat_statements columns
- b4deefc39b93 15.0 landed
- 3b57d5af7435 14.0 landed
-
Fix wording in description of pg_stat_statements.toplevel
- f9e6d00df029 14.0 landed
-
Mention that toplevel is part of pg_stat_statements key.
- 7531fcb1fcf5 14.0 landed
-
adjust query id feature to use pg_stat_activity.query_id
- 9660834dd8bf 14.0 landed
-
Update copyright for 2021
- ca3b37487be3 14.0 cited
-
Take the statistics collector out of the loop for monitoring backends'
- b13c9686d084 8.2.0 cited