Add SHOW CLIENTS / SHOW SERVERS + Stats refactor and tests

[drdrsh]

This started as an effort to add SHOW CLIENTS to allow for attribution of server work to specific application_names. What ended up happening is a big refactor for the Pgcat stats after I encountered a bug where it seemed that cl_* stats seemed to wrongfully include admin clients, I wasn't able to fix that without changing a lot of things hence, this refactor.

The major changes here are

• Introduction of SHOW CLIENTS and SHOW SERVERS queries, both are backed by an ArcSwap HashMap that gets updated by the stats collector every second.
• Introduction of client_register and server_register events that mark a clear starting point for tracking stats for both client and server. We collect some information about clients/servers in that point and continue to use that information for subsequent events.
• Using Anonymous structs in the Enum variants to attach relevant information to each event.
• Making a distinction between server_id which is a random identifier we generate for each server connection and the process_id which is the Postgres Backend PID used for cancel query requests. Prior to this PR both concepts were one and the same.
• Report client errors using stats system, so errors like failing to obtain a connection from a pool or a server getting banned while client is talking to it.
• Some changes to the placement of event reporting calls
• Integration tests for stats

[drdrsh]

This walks around the need to send app_name, username and pool_name to send these information with every client event.

We can do that anyway to keep the stats interface stateless

[drdrsh]

Being able to know which applications have the most active clients will be a very useful

[levkk]

You don't need a mutex, the stats collector is not multi-threaded. I think you should be able to deduce these stats from the structures we have below in collect().

[drdrsh]

This lock here is needed because this hashmap is also read by admin.rs to construct SHOW CLIENTS

Access pattern for this data structure is mostly writes (updating client stats), reads are only triggered by SHOW CLIENTS so my strategy here is to acquire the read lock enough to clone the hashmap, unlock and then use it. I am open for suggestions

[levkk]

Checkout how we do it for regular stats. The lock exists but it's only used by the collect() loop. Clients use channels instead which I believe to be faster because they should be lock-free.

[drdrsh]

We register any clients that connects, even if it does not perform queries. As such, we must also deregister any client that disconnects.

[levkk]

We're going back and forth on this one I think, didn't we remove this call at some point?

[drdrsh]

I think this was server_idle, I don't recall we touched client_disconnecting.

[drdrsh]

I feel a bit more confident about that stats after the refactor, having tracking objects for individual connections + registering all connections as early as possible makes it easier to reason about.

[drdrsh]

client_disconnecting has no use for last_address_id (Based on my reading of the code)

[zainkabani]

That's what I thought as well.

But it's used here, https://github.com/levkk/pgcat/blob/main/src/stats.rs#L398 to remove from the correct dict here, https://github.com/levkk/pgcat/blob/main/src/stats.rs#L456

[drdrsh]

We are overcounting transactions

[drdrsh]

Turns out that we are not.
This is how Pgbouncer does it, mostly.

Pgbouncer behavior

SELECT 1; # incremented total_xact_count and total_query_count by 1
BEGIN;    # incremented total_xact_count and total_query_count by 1 
SELECT 1; # incremented only total_query_count by 1
SELECT 2; # incremented only total_query_count by 1
ROLLBACK; # incremented only total_query_count by 1
SELECT 1; SELECT 2; SELECT 3; SELECT 4; SELECT 5; # increment both metrics by 5

Pgcat behavior

SELECT 1; # incremented total_xact_count and total_query_count by 1
BEGIN;    # incremented total_query_count by 1 
SELECT 1; # incremented only total_query_count by 1
SELECT 2; # incremented only total_query_count by 1
ROLLBACK; # incremented both metrics  by 1
SELECT 1; SELECT 2; SELECT 3; SELECT 4; SELECT 5; # increment both metrics by 5

[levkk]

I see, we're incrementing txn number at the end of the transaction instead of at the beginning, that's cool.

[levkk]

This is really cool

[zainkabani]

Let's make this either a struct or at least named parameters

[drdrsh]

I already made a change to my local branch to use anonymous struct

[drdrsh]

Errors are pooler errors encountered by the client. In this PR I introduce two kinds

• Checkout Failure
• Server Connection Banned while client is talking to it

[drdrsh]

This is a big change. So I am using a randomly generated number to refer to individual server connections and using that to store server stats.

The server_stats is a global object (spans across all pools in the process) and I figured it is not impossible that two PG backend PIDs my collide, so in this PR, server.process_id is used only for query cancellation. Stats will track a different number that is generated randomly like the one for the client

[drdrsh]

We didn't really checkout the connection here so we report an error instead of reporting a checkout time.

[drdrsh]

Client doesn't disconnect when server connection is banned. We send back an error message but also send ReadyForQuery message afterwards.

[drdrsh]

reporting client_disconnecting when the client is still connected would cause all of its subsequent events to be ignored and will cause us to undercount the number of connected clients. We should only call client_disconnecting when we know for fact the client is disconnecting which happens only in Drop

[drdrsh]

I will take another pass to clean up comments.
Will also add a bit more tests for stats (around cl_waiting and waiting times)

[drdrsh]

SHOW SERVERS

SHOW CLIENTS

[drdrsh]

These are no longer needed, we are using anonymous structs to attach information specific to each event.

[zain-kabani]

instead of adding only the important values to this struct could we instead use a more general struct like client_metadata which holds all the info we might need about a client and probably just use address for server info. This way if we ever want to add tagging to statsd we can do so really easily

[drdrsh]

That would require constructing the client metadata object on every call site. Sometimes all we have is just client_id

[zain-kabani]

This is what I used in my here, https://github.com/levkk/pgcat/blob/1d32eefe633572f25165670b1c84d56a486f2f2e/src/stats.rs#L48

[drdrsh]

The client_register event is the only place where we need to add meta data. The meta data is stored locally and the collector and we do a lookup on client_id every time an event comes

[zain-kabani]

The way I did it was just creating it once and then cloning it when needed (clone required cause of strings)

[drdrsh]

The equivalent of the client_metadata here would be the ClientInformation struct that we populated at client_register event

[drdrsh]

Example lookup
https://github.com/levkk/pgcat/blob/08af06d6402f31fb105ea01bf656667c7eff7099/src/stats.rs#L567-L573

I was trying as much as possible to avoid adding any overhead on the main thread when reporting stats. Cloning the metadata object for every stats call feel excessive to me.

If the goal is to allow us to bundle useful information about the client, then that is exactly what the PR does. But instead of constructing it over and over again, it is held locally in the collector

[drdrsh]

Client and server information objects
https://github.com/levkk/pgcat/blob/08af06d6402f31fb105ea01bf656667c7eff7099/src/stats.rs#L85-L128

[zain-kabani]

Ah okay yeah the look up pattern seems more optimal in this case since and we always have ids to reference what we need. nice!

[levkk]

Amazing stuff!