sql: create and fill crdb_internal.node_transaction_statistics table

[arulajmani]

Previously there was no way to query the transaction level metrics
collected by individual nodes. This patch adds this capability by
exposing a new internal table called
crdb_internal.node_transaction_statistics which is analagous to
crdb_internal.node_statement_statistics albeit for transactions.

Closes #53504

Release justification: low risk update to new functionality
Release note (sql change): A new crdb_internal table called
node_transaction_statistics is exposed as part of this change,
which allows users to query transaction metrics collected on a
particular node.

[cockroach-teamcity]

This change is 

[solongordon]

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

---

pkg/sql/crdb_internal.go, line 864 at r1 (raw file):

var crdbInternalTransactionStatisticsTable = virtualSchemaTable{
	comment: `finer grain transaction statistics (in-memory, not durable; local node only). ` +

nit: "finer-grained"

---

pkg/sql/crdb_internal.go, line 867 at r1 (raw file):

		`This table is wiped periodically (by default, at least every two hours)`,
	schema: `
CREATE  TABLE crdb_internal.node_transaction_statistics (

nit: two spaces after CREATE

---

pkg/sql/crdb_internal.go, line 868 at r1 (raw file):

	schema: `
CREATE  TABLE crdb_internal.node_transaction_statistics (
  node_id 					INT NOT NULL,

I wonder if we should also include the transaction hash here. It would be good to have some unique identifier per row. Otherwise it would be annoying to, say, write a query which returns all the statements for a particular transaction.

If so we should probably call it something other than transaction_id since that's already something different. Maybe transaction_key or just key?

---

pkg/sql/crdb_internal.go, line 878 at r1 (raw file):

  retry_lat_var 		FLOAT NOT NULL,
	commit_lat_avg 		FLOAT NOT NULL,
  commit_lat_var 		FLOAT NOT NULL,

nit: doesn't quite line up

---

pkg/sql/crdb_internal.go, line 884 at r1 (raw file):

`,
	populate: func(ctx context.Context, p *planner, _ *dbdesc.Immutable, addRow func(...tree.Datum) error) error {
		if err := p.RequireAdminRole(ctx, "access application statistics"); err != nil {

We should actually make this require the new VIEWACTIVITY role privilege, not admin. Would you mind making that change for the statement version of this table too while you're here?

---

pkg/sql/crdb_internal.go, line 921 at r1 (raw file):

			// Now retrieve the per-txn stats proper.
			for _, txnKey := range txnKeys {
				// The key should exist, so it's okay to pass nil for the statementIDs

What if the statistics happen to get cleared while this function is running? I think you should make sure to handle the case where the key doesn't exist.

[arulajmani]

Addressed review comments, should be RFAL. Can't pinpoint why the pg_catalog test is failing because of this change.

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

---

pkg/sql/crdb_internal.go, line 868 at r1 (raw file):

Previously, solongordon (Solon) wrote…

I wonder if we should also include the transaction hash here. It would be good to have some unique identifier per row. Otherwise it would be annoying to, say, write a query which returns all the statements for a particular transaction.

If so we should probably call it something other than transaction_id since that's already something different. Maybe transaction_key or just key?

Added a key field. Didn't realize we had a key field for the statement version of this table.

---

pkg/sql/crdb_internal.go, line 878 at r1 (raw file):

Previously, solongordon (Solon) wrote…

nit: doesn't quite line up

Tabs and spaces haha, Fixed now!

---

pkg/sql/crdb_internal.go, line 884 at r1 (raw file):

Previously, solongordon (Solon) wrote…

We should actually make this require the new VIEWACTIVITY role privilege, not admin. Would you mind making that change for the statement version of this table too while you're here?

Added!

---

pkg/sql/crdb_internal.go, line 921 at r1 (raw file):

Previously, solongordon (Solon) wrote…

What if the statistics happen to get cleared while this function is running? I think you should make sure to handle the case where the key doesn't exist.

Good catch! Handled.

[arulajmani]

Looks like the pg_catalog stuff changed because there is a new comment on the table I'm adding.

[solongordon]

Is it weird that all those OIDs went down by 1? It would seem more expected for them to go up since you added something new but I don't really know how these things are generated.

Reviewed 3 of 9 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @solongordon)

[knz]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani and @solongordon)

---

pkg/sql/crdb_internal.go, line 121 at r2 (raw file):

		catconstants.CrdbInternalTxnStatsTableID:             crdbInternalTxnStatsTable,
		catconstants.CrdbInternalZonesTableID:                crdbInternalZonesTable,
		catconstants.CrdbInternalTransactionStatsTableID:     crdbInternalTransactionStatisticsTable,

nit: For future maintainability, it's better to maintain this dict ordered alphabetically.

---

pkg/sql/catalog/catconstants/constants.go, line 83 at r2 (raw file):

	CrdbInternalTxnStatsTableID
	CrdbInternalZonesTableID
	CrdbInternalTransactionStatsTableID

ditto

[arulajmani]

addressed the nits and updated the zip tests. TFTRs!

bors r=solongordon

[craig]

Build succeeded:

• GitHub CI (Cockroach)