server: use stats activity tables on sql stats endpoint

[maryliag]

With the new sql stats tables that contain the top 500 rows based on most used column, this PR updates the calls on the sql stats endpoint to use the new flow:

flowchart TD;
    A[Compare the TS on ACTIVITY Table with the Requested TS] --> B{Is the requested time \nperiod completely \non the table?}
    B -- Yes --> C[SELECT on ACTIVITY table]
    C --> D{Had results?}
    D -- Yes --> E[Return RESULTS]
    D -- No --> F[SELECT on PERSISTED table]
    B -- No ----> F
    F --> G{Had results?}
    G -- Yes --> E
    G -- No --> H[SELECT on COMBINED table]
    H --> E

Loading

Part Of: #101948

A following PR will deal when selecting a column that is not one of the ones selected to generate the activity tables.

Release note (performance improvement): SQL Activity endpoints now use first a table with the top data for the most used cases. If there is no data available, it used the previous flow with persisted data and if that is also empty, uses in-memory.

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[j82w]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

---

-- commits line 9 at r3:
Can a test be added to make sure the fallback logic works?

---

pkg/server/combined_statement_stats.go line 83 at r3 (raw file):

	var err error
	showInternal := SQLStatsShowInternal.Get(&settings.SV)
	whereClause, orderAndLimit, args := getCombinedStatementsQueryClausesAndArgs(

getCombinedStatementsQueryClausesAndArgs should be updated so the order by and limit use the new columns in the activity table for the better perf.

---

pkg/server/combined_statement_stats.go line 97 at r3 (raw file):

		activityHasAllData, err = activityTablesHaveFullData(ctx, ie, testingKnobs, reqStartTime)
		if err != nil {
			return nil, serverError(ctx, err)

Should this just log the error and set activityHasAllData to false? That way even if the check fails the entire request can possibly succeed.

---

pkg/server/combined_statement_stats.go line 177 at r3 (raw file):

) (result bool, err error) {
	var auxDate time.Time
	dateFormat := "2006-01-02"

Why does it only have the date and not the time?

---

pkg/server/combined_statement_stats.go line 178 at r3 (raw file):

	var auxDate time.Time
	dateFormat := "2006-01-02"
	auxDate, err = time.Parse(dateFormat, "1970-01-01")

Should this be?
auxDate, err = time.Parse(dateFormat, reqStartTime)

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w)

---

-- commits line 9 at r3:

Previously, j82w (Jake) wrote…

Can a test be added to make sure the fallback logic works?

The current tests that make calls to the endpoints already cover that case, we do have check when there is only in memory and then persisted. The results are as expected.

---

pkg/server/combined_statement_stats.go line 83 at r3 (raw file):

Previously, j82w (Jake) wrote…

getCombinedStatementsQueryClausesAndArgs should be updated so the order by and limit use the new columns in the activity table for the better perf.

All changes related to columns selection will come on a following PR, because I also need to include the cases where the column is not on the activity, and it was getting to big for one PR.

Also, since we're combining things, does it have a better performance on an aggregated column vs the statistics column? I thought the index would not matter at that point. Do we even have an aggregation for each specific column?

---

pkg/server/combined_statement_stats.go line 97 at r3 (raw file):

Previously, j82w (Jake) wrote…

Should this just log the error and set activityHasAllData to false? That way even if the check fails the entire request can possibly succeed.

Done

---

pkg/server/combined_statement_stats.go line 177 at r3 (raw file):

Previously, j82w (Jake) wrote…

Why does it only have the date and not the time?

Fixed

---

pkg/server/combined_statement_stats.go line 178 at r3 (raw file):

Previously, j82w (Jake) wrote…

Should this be?
auxDate, err = time.Parse(dateFormat, reqStartTime)

Actually should be now, because this is used for the case where you don't have any data on the table, so it returns this on the coalesce, meaning the min date is now. I don't want to be the same as the request because I compare at the end and if they're the same it means I have data, which is not the case.
I changed to use now

[j82w]

Reviewable status: complete! 1 of 0 LGTMs obtained

[maryliag]

TFTR!
bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 17128ce to blathers/backport-release-23.1-102026: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

error creating merge commit from 17128ce to blathers/backport-release-23.1.0-102026: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.0 failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}