sql,roachpb: add plan hash correct value to persisted stats

[maryliag]

Previously, we didn't have the plan hash/gist values, so
a dummy value was being used instead. Now that we have the value,
this commit uses those values to be corrected stored.
The Plan Hash is saved on its own column and is part of a
statement key. A plan gist is a string saved in the metadata
and can later on converted back into a logical plan.

Partially addresses #72129

Release note (sql change): Saving plan hash/gist to the Statements
persisted stats.

[cockroach-teamcity]

This change is 

[Azhng]

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

---

pkg/roachpb/app_stats.go, line 57 at r2 (raw file):

		fnv.Add('E')
	}
	fnv.Add(planHash)

I think the statement fingerprint ID should be constructed without the plan_hash. Otherwise, it would be quite difficult to identify different plans used by the statement fingerprint, e.g.:

SELECT
  statistics ->> 'plan_gist'
FROM
  system.statement_statistics
WHERE
  fingerprint_id = $1

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go, line 53 at r2 (raw file):

  "vec":         {{.Bool}},
  "fullScan":    {{.Bool}},
  "planGists":   {{.StringArray}}

Hmm, so in the proto definition, you added the plan_gist in the StatementStatistics message, which is corresponding to the statistics column (few lines below). metadata column is mapped to StatementStatisticsKey protobuf message. So introducing the planGist into the metadata might feel a bit weird since now the encoder/decoder responsible for the metadata that's not present in the StatementStatisticsKey protobuf message.

I think we have two options here, we can either:

1. put planGist in the statistics column and it can stay in StatementStatistics protobuf message
2. put planGist in the metadata column and it needs to be moved from StatementStatistics protobuf message into StatementStatisticsKey proto message.

[maryliag]

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

---

pkg/roachpb/app_stats.go, line 57 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

I think the statement fingerprint ID should be constructed without the plan_hash. Otherwise, it would be quite difficult to identify different plans used by the statement fingerprint, e.g.:

SELECT
  statistics ->> 'plan_gist'
FROM
  system.statement_statistics
WHERE
  fingerprint_id = $1

Done.

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go, line 53 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Hmm, so in the proto definition, you added the plan_gist in the StatementStatistics message, which is corresponding to the statistics column (few lines below). metadata column is mapped to StatementStatisticsKey protobuf message. So introducing the planGist into the metadata might feel a bit weird since now the encoder/decoder responsible for the metadata that's not present in the StatementStatisticsKey protobuf message.

I think we have two options here, we can either:

1. put planGist in the statistics column and it can stay in StatementStatistics protobuf message
2. put planGist in the metadata column and it needs to be moved from StatementStatistics protobuf message into StatementStatisticsKey proto message.

I decided to go with option 1, so on the next step I can use the builtin to merge stats instead of creating a new one for metadata. And it could be misleading having plan gist as part of something called Key without actually being part of the Key :)

[Azhng]

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

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/testutils.go, line 148 at r3 (raw file):

		val.SetBool(data.Bool)
	case reflect.Slice:
		for _, randInt := range data.IntArray {

Hmm I think some additional code here needs to be added to handle the new .StringArray type

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go, line 100 at r3 (raw file):

         },
         "nodes": [{{joinInts .IntArray}}],
         "planGists":   {{.StringArray}}

Hmm you might need to define a template function here (similar to joinInts defined a line above, and you also need to put that callback into the template engine) so the template engine knows to covert golang's string array into a textual json string.

[maryliag]

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

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/testutils.go, line 148 at r3 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Hmm I think some additional code here needs to be added to handle the new .StringArray type

Done

---

pkg/sql/sqlstats/persistedsqlstats/testdata/logical_plan_sampling_for_explicit_txn, line 31 at r4 (raw file):

should-sample-logical-plan db=defaultdb implicitTxn=true fingerprint=SELECT%_
----
true

@Azhng looks like the changes are triggering new sampling of plan. I changed here on the file the places that were failing if kept as false. The addition of plan hash as part of the key could be what is causing the plan getting sampled more often?

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go, line 100 at r3 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Hmm you might need to define a template function here (similar to joinInts defined a line above, and you also need to put that callback into the template engine) so the template engine knows to covert golang's string array into a textual json string.

Done.

[Azhng]

Reviewed 7 of 17 files at r1, 2 of 6 files at r3, 3 of 5 files at r4, 5 of 11 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

---

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go, line 52 at r5 (raw file):

	implicitTxn    bool
	database       string
	planHash       uint64

Ah this is my bad, I should have left a comment here. The planKey here is used to determine if we should tell the optimizer to build a full EXPLAIN plan. planKey is embedded into the stmtKey to form the full key that uniquely identifies a statement stats. In a stats Container, there's a plan sample cache and it's updated each time a plan is sampled.

Let's move the planHash into stmtKey struct, this is so that plan sampling would still work correctly.

Hmm perhaps the name is actually quite confusing, since

composition	what it uniquely identifies
[anonymizedStmt, failed, implicitTxn, database]	statement stats of a specific statement fingerprint ID
[anonymizedStmt, failed, implicitTxn, database, transaction fingerprint id]	statement stats of a statement fingerprint ID that were part of a specific transaction fingerprint
[anonymizedStmt, failed, implicitTxn, database, planHash]	statement stats of a plan of a specific statement fingerprint ID
[anonymizedStmt, failed, implicitTxn, database, txn fingerprint ID, planHash]	statement stats of a plan of a specific statement fingerprint ID that were executed as part of a specific transaction fingerprint ID

Since, planKey actually doesn't actually identifies a plan, this name is quite misleading. Since it's used as a heuristics to determine whether a plan sampling should happen, it's probably more fitting to call it something like sampledPlanKey or statementSignature

---

pkg/sql/sqlstats/persistedsqlstats/testdata/logical_plan_sampling_for_explicit_txn, line 31 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

@Azhng looks like the changes are triggering new sampling of plan. I changed here on the file the places that were failing if kept as false. The addition of plan hash as part of the key could be what is causing the plan getting sampled more often?

Hmm this is bad 🤔 . Plan sampling means we are building a full EXPLAIN plan, which is really expensive. I think this should be fixable (see comment above) and we should preserve the plan sampling behavior.

[maryliag]

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

---

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go, line 52 at r5 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Ah this is my bad, I should have left a comment here. The planKey here is used to determine if we should tell the optimizer to build a full EXPLAIN plan. planKey is embedded into the stmtKey to form the full key that uniquely identifies a statement stats. In a stats Container, there's a plan sample cache and it's updated each time a plan is sampled.

Let's move the planHash into stmtKey struct, this is so that plan sampling would still work correctly.

Hmm perhaps the name is actually quite confusing, since

composition	what it uniquely identifies
[anonymizedStmt, failed, implicitTxn, database]	statement stats of a specific statement fingerprint ID
[anonymizedStmt, failed, implicitTxn, database, transaction fingerprint id]	statement stats of a statement fingerprint ID that were part of a specific transaction fingerprint
[anonymizedStmt, failed, implicitTxn, database, planHash]	statement stats of a plan of a specific statement fingerprint ID
[anonymizedStmt, failed, implicitTxn, database, txn fingerprint ID, planHash]	statement stats of a plan of a specific statement fingerprint ID that were executed as part of a specific transaction fingerprint ID

Since, planKey actually doesn't actually identifies a plan, this name is quite misleading. Since it's used as a heuristics to determine whether a plan sampling should happen, it's probably more fitting to call it something like sampledPlanKey or statementSignature

Done

---

pkg/sql/sqlstats/persistedsqlstats/testdata/logical_plan_sampling_for_explicit_txn, line 31 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Hmm this is bad 🤔 . Plan sampling means we are building a full EXPLAIN plan, which is really expensive. I think this should be fixable (see comment above) and we should preserve the plan sampling behavior.

The change in key fixed this issue!

[xinhaoz]

---

pkg/server/combined_statement_stats.go, line 143 at r7 (raw file):

					FROM crdb_internal.statement_statistics
					%s
					GROUP BY

I'm a little confused by the change of query here and might be missing something. We're already grouping by all of these columns in the view. If we're no longer trying to group by the plan_hash, I think we should remove that from the view's query. Is that what this new query is supposed to do? Also if we're moving the limit and AOST calause outside its previous function, we have to add it tot he transactions query as well.

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @xinhaoz)

---

pkg/server/combined_statement_stats.go, line 143 at r7 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

I'm a little confused by the change of query here and might be missing something. We're already grouping by all of these columns in the view. If we're no longer trying to group by the plan_hash, I think we should remove that from the view's query. Is that what this new query is supposed to do? Also if we're moving the limit and AOST calause outside its previous function, we have to add it tot he transactions query as well.

The view has separate by plan hash, on this endpoint ( which is used on the tables), we're grouping ignoring the plan hash, but on the details page we want to keep separate just like the original view (which will be a new endpoint). So if I change the original view to ignore the plan hash, I won't be able to split them for the details.

Good catch about the Txn, I made the adjustments

[Azhng]

Nice! Just small style nits left.

Reviewed 1 of 7 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @xinhaoz)

---

pkg/server/combined_statement_stats.go, line 94 at r8 (raw file):

func getFilterAndParams(
	start, end *time.Time, limit int64, testingKnobs *sqlstats.TestingKnobs,
) (string, string, []interface{}) {

small (optional) nit: personally I prefer to name the return variables when there are more than two of them in the function. getFilterAndParams implies that there are two return vars and one is filter and the other one is param. Having a name here for each returned vars might clear things up. It's up to you.

---

pkg/server/combined_statement_stats.go, line 129 at r8 (raw file):

		`SELECT
        fingerprint_id,
				transaction_fingerprint_id,

small style nit: seems like the vertical indentation is misaligned. (might cuz mixing tabs or space?)

---

pkg/server/combined_statement_stats.go, line 143 at r8 (raw file):

        aggregated_ts,
				aggregation_interval
      %s`, whereClause, orderAndLimit)

same minor nits on the indents

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng and @xinhaoz)

---

pkg/server/combined_statement_stats.go, line 94 at r8 (raw file):

Previously, Azhng (Archer Zhang) wrote…

small (optional) nit: personally I prefer to name the return variables when there are more than two of them in the function. getFilterAndParams implies that there are two return vars and one is filter and the other one is param. Having a name here for each returned vars might clear things up. It's up to you.

Made some changes, also added comment :)

---

pkg/server/combined_statement_stats.go, line 129 at r8 (raw file):

Previously, Azhng (Archer Zhang) wrote…

small style nit: seems like the vertical indentation is misaligned. (might cuz mixing tabs or space?)

fixed

---

pkg/server/combined_statement_stats.go, line 143 at r8 (raw file):

Previously, Azhng (Archer Zhang) wrote…

same minor nits on the indents

fixed

[xinhaoz]

---

pkg/server/combined_statement_stats.go, line 143 at r7 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

The view has separate by plan hash, on this endpoint ( which is used on the tables), we're grouping ignoring the plan hash, but on the details page we want to keep separate just like the original view (which will be a new endpoint). So if I change the original view to ignore the plan hash, I won't be able to split them for the details.

Good catch about the Txn, I made the adjustments

Ah I see. Thanks for the explanation!

[maryliag]

TFTR
bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)