ui: statements page should disambiguate statements based on database

[jordanlewis]

Currently, statements on the statement page do not seem to be bucketed by database. This makes it impossible to distinguish whether a statement on a table is from one database or another.

Repro:

root@127.0.0.1:56758/defaultdb> create table a (a int primary key);
CREATE TABLE

Time: 6.559ms

root@127.0.0.1:56758/defaultdb> insert into a values(1);
INSERT 1

Time: 5.411ms

root@127.0.0.1:56758/defaultdb> create database foo;
CREATE DATABASE

Time: 4.107ms

root@127.0.0.1:56758/defaultdb> use foo;
SET

Time: 402µs

root@127.0.0.1:56758/foo> create table a (a int primary key);
CREATE TABLE

Time: 5.812ms

root@127.0.0.1:56758/foo> insert into a values(1);
INSERT 1

Time: 6.249ms

Actual result:

Expected: the two INSERT statements should not be in the same bucket, since they're on different tables, despite the tables having the same name.

[knz]

@jordanlewis @piyush-singh we do not currently bucket statement statistics based on the value of the "current database". It's not just a matter of UI, the back-end collection must be changed as well.

[piyush-singh]

cc @couchand another wrinkle for persistence. I can't believe this didn't occur to me earlier, thanks for flagging @jordanlewis !

Andrew, this makes sense to me as a natural extension to how we handle statement stats/how users would want to filter. We can discuss whether we want to prioritize this for 2.2 at our next sprint planning. Adding as a user story to the persistence AT record.

[couchand]

As @knz alluded to but may not have made completely clear, it's not just about the storage of the stats but about the way that we collect them. I would expect getting this right to be more complicated than we can reasonably expect to do in 2.2, since it requires a lot more understanding of the semantics of what's going on that we currently kind of pretend don't exist.

Just to extend the example a bit, if you added a statement like:

insert into foo.a values(1);

The results would be even more confusing/misleading/wrong -- we'd never combine the same statement if one instance of it used fully-qualified names.

I'm happy to brainstorm ideas to address these issues, but they are necessarily much more complicated than just the UI.

[jordanlewis]

Hey @couchand, totally agreed that this kind of example makes things more complex! But IMO a quick fix would be to just bucket by "current database" and ignore trying to coalesce things more correctly like your example. The use case (that I didn't link to hear because it was in a private repo) was just that there's a product that has several workloads running at once against different databases and they want to see which workload is doing what.

[jordanlewis]

Here*. ugh

[couchand]

My point may not have been clear with my example. My point was that, if you want to use database as a criteria to filter/compare on (like you can today with app_name), it's not going to do what you want if it's based solely on the current database setting.

[jordanlewis]

Ah, right! I forgot that it's also bucketed by app name. I wonder if the problem would have gone away if I had suggested that to this user.

[knz]

My first comment above was a matter-of-fact comment on the technicalities of solving the problem. I did not back then express my opinion about this feature ask. Let me do that now.

I do not like it.

(It makes no sense and we already have a solution anyway. read on.)

The use case (that I didn't link to hear because it was in a private repo) was just that there's a product that has several workloads running at once against different databases and they want to see which workload is doing what.
Ah, right! I forgot that it's also bucketed by app name. I wonder if the problem would have gone away if I had suggested that to this user.

That's the thing. The one true and tried way to organize workloads in a pg-like db is application_name, not the current databaase. We also use the app_name in many other places as a identification string (table audits, execution log, SHOW SESSIONS/QUERIES, probably others by now).
The user should have been advised from the start to use that, and if they had we would not be having this discussion now.

Now on why I don't like doing anything with the db name.

1. it doesn't make sense (like Andrew noted).

I'll start with the same argument as Andrew. One "application" can use multiple databases, going so far as having the same client session switch between values of the current db setting, or using cross-db transactions. There is no "identity" of an app in its current db session var.

2. it's going to create poor UX.

Really, it's inconsistent with the other places where we identify applications (table audits, exec log etc). If we start to identify apps by the current db in the UI, we need to start doing so in every other places too.

All the while the pg folk have already determined that app_name is the way to go and that the cur db session var is worthless / not interesting.

So, IMHO, no. Let's not make this change and instead properly document how to use app_name.

[piyush-singh]

FWIW I read up on the customer conversation Jordan was involved in back in December. Robert correctly suggested to the customer that they try using app_name. From the conversation, it seems like they saw that as an acceptable solution and were going to try it out. I can follow up and confirm that they managed to set app_name correctly via hibernate and that this solves their need. I'll close this ticket if that is indeed the case.

[ajwerner]

ideally we'd rewrite queries to use numerical identifiers for all of their references and use that for fingerprints. That would also give us a fingerprint that we could render back to the user in a nice way if we wanted. Ah, that'd be great. This is yet another reason to need the ability to get fine-grained information out of the optbuilder to rewrite ASTs.

[maryliag]

Summary:

• include database into the fingerprint and sql view (crdb_internal.node_statement_statistics)
• show the database name on the Statements tables and details (transaction pages too?)
• create a new filter based on database for both Statements and Transactions Page

@Annebirzin can you update figma with the new field? Not sure if we want a new column or show some other way
(we might consider adding the possibility of the user choosing the columns to show, if we keep following this path of adding more and more columns to those tables)

[Annebirzin]

@maryliag I've explored some initial designs for adding databases to statements here :

option 1: add database column with ability to hide/show columns AND add filter by database
cons:

• more columns
• saving column preference could be complicated

option 2: 'group by' database AND filter by database (no database column)
cons:

• not sure this scales for lots of dbs (how would collapse/expand groups work with pagination)
• if there is no collapse/expand, could scale be an issue? (maybe solved with filtering)

Let me know any thoughts/ideas

[maryliag]

option 1:
as a user I think I would like this one more, and we could have some hidden column (like database) hidden by default and the user can select if they want.
adding just a new column is definitely the easiest solution, but the extra save for column selection would take more work, but I have a feeling if we want to keep on this path of adding more information, it would be better do this sooner then later

option2: the collapse part seems to be tricky, maybe we could have a row for the database name, but not necessary have it collapse? I need to investigate a little of how this can be done (and check how it works with pagination as you mentioned)