Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#159] Add new pool metric pgagroal_query_count and Query rate panel #161

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

An-DJ
Copy link
Contributor

@An-DJ An-DJ commented Jul 25, 2021

Issue

#159

Task

  • Add pgagroal_query_count which characterizes the number of queries.
  • Add new panel Query rate in Grafana dashboard,

@jesperpedersen
Copy link
Collaborator

This would count all client requests as a query - it would be better to just count the actual SQL queries.

See https://www.postgresql.org/docs/devel/protocol-message-formats.html for the message protocol - you will need support for the simple query and the prepared query format. You can use https://github.com/jesperpedersen/pgprtdbg for protocol debugging.

You can likely reuse code from pipeline_transaction.c to track message boundaries, but we need guard against this extra overhead - maybe a statistics configuration variable with a default of on.

Only pipeline_session.c and pipeline_transaction.c needs to support this -- pipeline_perf.c won't support extended statistics.

@An-DJ
Copy link
Contributor Author

An-DJ commented Jul 29, 2021

This would count all client requests as a query - it would be better to just count the actual SQL queries.

See https://www.postgresql.org/docs/devel/protocol-message-formats.html for the message protocol - you will need support for the simple query and the prepared query format. You can use https://github.com/jesperpedersen/pgprtdbg for protocol debugging.

You can likely reuse code from pipeline_transaction.c to track message boundaries, but we need guard against this extra overhead - maybe a statistics configuration variable with a default of on.

Only pipeline_session.c and pipeline_transaction.c needs to support this -- pipeline_perf.c won't support extended statistics.

Thanks for your guidance. I am implementing the filtering of SQL queries by protocol key info. I will push newer code soon.

@An-DJ
Copy link
Contributor Author

An-DJ commented Aug 2, 2021

@jesperpedersen I have check the message kind in Simple Query and Extended Query(prepare-statement).

  • For the SimpleQuery, a query can be created by a Q message.
  • For the PrepareQuery, a prepare query can be created by a P message which is used to parse the unsettled sql. Then a execute query can be created by a E message which is used to execute the settled sql.

So the query_count should be increased by 'Q' 'P' and 'E'. The newer commit includes that implementation.

@@ -97,6 +97,7 @@ performance_client(struct ev_loop *loop, struct ev_io *watcher, int revents)
wi = (struct worker_io*)watcher;

status = pgagroal_read_socket_message(wi->client_fd, &msg);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

White space

@@ -220,6 +220,11 @@ session_client(struct ev_loop *loop, struct ev_io *watcher, int revents)
{
if (likely(msg->kind != 'X'))
{
if (msg->kind == 'Q' || msg->kind == 'P' || msg->kind == 'E')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P is Parse which we shouldn't count as a query, as B (Bind) won't need it after 5 statements.

Lets start with Q and E in this category.

@@ -236,6 +236,11 @@ transaction_client(struct ev_loop* loop, struct ev_io* watcher, int revents)
{
if (likely(msg->kind != 'X'))
{
if (msg->kind == 'Q' || msg->kind == 'P' || msg->kind == 'E')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@jesperpedersen
Copy link
Collaborator

You are missing the reset part for the new metric

@An-DJ
Copy link
Contributor Author

An-DJ commented Aug 3, 2021

You are missing the reset part for the new metric

Done. I found that a 'P' message(parse) can be sent by prepare statement, and 'B','E'... messages can be sent by execute statement. Maybe the parsing phase should not be considered as query. I have removed it just now.

@jesperpedersen jesperpedersen merged commit f1bbe8a into agroal:master Aug 4, 2021
@jesperpedersen
Copy link
Collaborator

Thanks for your contribution !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants