Skip to content

Commit

Permalink
sql: revert formatting all query statements
Browse files Browse the repository at this point in the history 
Previously, all statements were formatted prior to being sent to the
frontend. However, the formatting was causing statements queries from
the frontend to run noticeably more slowly. This change removes the
logic that formats all queries, but keeps the addition of the new
builtin function prettify_statement.

Release note (sql change): statements are no longer formatted prior to
being sent to the UI, but the new builtin function remains.
  • Loading branch information
@THardy98
THardy98 committed Jan 24, 2022
1 parent 9e6c361 commit c02b3b1
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 130 deletions.
122 changes: 32 additions & 90 deletions pkg/ccl/serverccl/statusccl/tenant_status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/spanconfig"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catconstants"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sqlstats"
"github.com/cockroachdb/cockroach/pkg/sql/tests"
"github.com/cockroachdb/cockroach/pkg/testutils"
Expand Down Expand Up @@ -116,35 +115,19 @@ func TestTenantCannotSeeNonTenantStats(t *testing.T) {
tenantStatusServer := tenant.StatusServer().(serverpb.SQLStatusServer)

type testCase struct {
stmt string
formattedStmt string
fingerprint string
formattedFingerprint string
stmt string
fingerprint string
}

testCaseTenant := []testCase{
{stmt: `CREATE DATABASE roachblog_t`},
{stmt: `SET database = roachblog_t`},
{stmt: `CREATE TABLE posts_t (id INT8 PRIMARY KEY, body STRING)`},
{
stmt: `CREATE DATABASE roachblog_t`,
formattedStmt: "CREATE DATABASE roachblog_t\n",
},
{
stmt: `SET database = roachblog_t`,
formattedStmt: "SET database = roachblog_t\n",
},
{
stmt: `CREATE TABLE posts_t (id INT8 PRIMARY KEY, body STRING)`,
formattedStmt: "CREATE TABLE posts_t (id INT8 PRIMARY KEY, body STRING)\n",
},
{
stmt: `INSERT INTO posts_t VALUES (1, 'foo')`,
fingerprint: `INSERT INTO posts_t VALUES (_, '_')`,
formattedStmt: "INSERT INTO posts_t VALUES (1, 'foo')\n",
formattedFingerprint: "INSERT INTO posts_t VALUES (_, '_')\n",
},
{
stmt: `SELECT * FROM posts_t`,
formattedStmt: "SELECT * FROM posts_t\n",
stmt: `INSERT INTO posts_t VALUES (1, 'foo')`,
fingerprint: `INSERT INTO posts_t VALUES (_, '_')`,
},
{stmt: `SELECT * FROM posts_t`},
}

for _, stmt := range testCaseTenant {
Expand All @@ -156,28 +139,14 @@ func TestTenantCannotSeeNonTenantStats(t *testing.T) {
require.NoError(t, err)

testCaseNonTenant := []testCase{
{stmt: `CREATE DATABASE roachblog_nt`},
{stmt: `SET database = roachblog_nt`},
{stmt: `CREATE TABLE posts_nt (id INT8 PRIMARY KEY, body STRING)`},
{
stmt: `CREATE DATABASE roachblog_nt`,
formattedStmt: "CREATE DATABASE roachblog_nt\n",
},
{
stmt: `SET database = roachblog_nt`,
formattedStmt: "SET database = roachblog_nt\n",
},
{
stmt: `CREATE TABLE posts_nt (id INT8 PRIMARY KEY, body STRING)`,
formattedStmt: "CREATE TABLE posts_nt (id INT8 PRIMARY KEY, body STRING)\n",
},
{
stmt: `INSERT INTO posts_nt VALUES (1, 'foo')`,
fingerprint: `INSERT INTO posts_nt VALUES (_, '_')`,
formattedStmt: "INSERT INTO posts_nt VALUES (1, 'foo')\n",
formattedFingerprint: "INSERT INTO posts_nt VALUES (_, '_')\n",
},
{
stmt: `SELECT * FROM posts_nt`,
formattedStmt: "SELECT * FROM posts_nt\n",
stmt: `INSERT INTO posts_nt VALUES (1, 'foo')`,
fingerprint: `INSERT INTO posts_nt VALUES (_, '_')`,
},
{stmt: `SELECT * FROM posts_nt`},
}

pgURL, cleanupGoDB := sqlutils.PGUrl(
Expand Down Expand Up @@ -230,22 +199,14 @@ func TestTenantCannotSeeNonTenantStats(t *testing.T) {
err = serverutils.GetJSONProto(nonTenant, path, &nonTenantCombinedStats)
require.NoError(t, err)

checkStatements := func(t *testing.T, tc []testCase, actual *serverpb.StatementsResponse, combined bool) {
checkStatements := func(t *testing.T, tc []testCase, actual *serverpb.StatementsResponse) {
t.Helper()
var expectedStatements []string
for _, stmt := range tc {
var expectedStmt = stmt.stmt
if combined {
expectedStmt = stmt.formattedStmt
}
if stmt.fingerprint != "" {
if combined {
expectedStmt = stmt.formattedFingerprint
} else {
expectedStmt = stmt.fingerprint
}
expectedStmt = stmt.fingerprint
}

expectedStatements = append(expectedStatements, expectedStmt)
}

Expand All @@ -272,14 +233,14 @@ func TestTenantCannotSeeNonTenantStats(t *testing.T) {

// First we verify that we have expected stats from tenants.
t.Run("tenant-stats", func(t *testing.T) {
checkStatements(t, testCaseTenant, tenantStats, false)
checkStatements(t, testCaseTenant, tenantCombinedStats, true)
checkStatements(t, testCaseTenant, tenantStats)
checkStatements(t, testCaseTenant, tenantCombinedStats)
})

// Now we verify the non tenant stats are what we expected.
t.Run("non-tenant-stats", func(t *testing.T) {
checkStatements(t, testCaseNonTenant, &nonTenantStats, false)
checkStatements(t, testCaseNonTenant, &nonTenantCombinedStats, true)
checkStatements(t, testCaseNonTenant, &nonTenantStats)
checkStatements(t, testCaseNonTenant, &nonTenantCombinedStats)
})

// Now we verify that tenant and non-tenant have no visibility into each other's stats.
Expand Down Expand Up @@ -313,29 +274,10 @@ func TestTenantCannotSeeNonTenantStats(t *testing.T) {
func testResetSQLStatsRPCForTenant(
ctx context.Context, t *testing.T, testHelper *tenantTestHelper,
) {

type testCase struct {
stmt string
formattedStmt string
}
stmts := []testCase{
{
stmt: "SELECT 1",
formattedStmt: "SELECT 1\n",
},
{
stmt: "SELECT 1, 1",
formattedStmt: "SELECT 1, 1\n",
},
{
stmt: "SELECT 1, 1, 1",
formattedStmt: "SELECT 1, 1\n",
},
}

var expectedStatements []string
for _, tc := range stmts {
expectedStatements = append(expectedStatements, tc.formattedStmt)
stmts := []string{
"SELECT 1",
"SELECT 1, 1",
"SELECT 1, 1, 1",
}

testCluster := testHelper.testCluster()
Expand Down Expand Up @@ -365,8 +307,8 @@ func testResetSQLStatsRPCForTenant(
}()

for _, stmt := range stmts {
testCluster.tenantConn(randomServer).Exec(t, stmt.stmt)
controlCluster.tenantConn(randomServer).Exec(t, stmt.stmt)
testCluster.tenantConn(randomServer).Exec(t, stmt)
controlCluster.tenantConn(randomServer).Exec(t, stmt)
}

if flushed {
Expand All @@ -383,7 +325,7 @@ func testResetSQLStatsRPCForTenant(

require.NotEqual(t, 0, len(statsPreReset.Statements),
"expected to find stats for at least one statement, but found: %d", len(statsPreReset.Statements))
ensureExpectedStmtFingerprintExistsInRPCResponse(t, expectedStatements, statsPreReset, "test")
ensureExpectedStmtFingerprintExistsInRPCResponse(t, stmts, statsPreReset, "test")

_, err = status.ResetSQLStats(ctx, &serverpb.ResetSQLStatsRequest{
ResetPersistedStats: true,
Expand Down Expand Up @@ -420,7 +362,7 @@ func testResetSQLStatsRPCForTenant(
})
require.NoError(t, err)

ensureExpectedStmtFingerprintExistsInRPCResponse(t, expectedStatements, statsFromControlCluster, "control")
ensureExpectedStmtFingerprintExistsInRPCResponse(t, stmts, statsFromControlCluster, "control")
})
}
}
Expand Down Expand Up @@ -607,10 +549,10 @@ SET DATABASE=test_db1;
SELECT * FROM test;
`)

getCreateStmtQuery := fmt.Sprintf(`
SELECT prettify_statement(indexdef, %d, %d, %d)
FROM pg_catalog.pg_indexes
WHERE tablename = 'test' AND indexname = $1`, tree.ConsoleLineWidth, tree.PrettyAlignAndDeindent, tree.UpperCase)
getCreateStmtQuery := `
SELECT indexdef
FROM pg_catalog.pg_indexes
WHERE tablename = 'test' AND indexname = $1`

// Get index usage stats and assert expected results.
resp := getTableIndexStats(testHelper, "test_db1")
Expand Down
10 changes: 2 additions & 8 deletions pkg/server/combined_statement_stats.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,18 +116,12 @@ func collectCombinedStatements(
transaction_fingerprint_id,
app_name,
aggregated_ts,
jsonb_set(
metadata,
array['query'],
to_jsonb(
prettify_statement(metadata ->> 'query', %d, %d, %d)
)
),
metadata,
statistics,
sampled_plan,
aggregation_interval
FROM crdb_internal.statement_statistics
%s`, tree.ConsoleLineWidth, tree.PrettyAlignAndDeindent, tree.UpperCase, whereClause)
%s`, whereClause)

const expectedNumDatums = 8

Expand Down
8 changes: 3 additions & 5 deletions pkg/server/index_usage_stats.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,23 +223,21 @@ func getTableIndexUsageStats(

q := makeSQLQuery()
// TODO(#72930): Implement virtual indexes on index_usages_statistics and table_indexes
q.Append(`SELECT
q.Append(`
SELECT
ti.index_id,
ti.index_name,
ti.index_type,
total_reads,
last_read,
prettify_statement(indexdef, $, $, $)
indexdef
FROM crdb_internal.index_usage_statistics AS us
JOIN crdb_internal.table_indexes AS ti ON us.index_id = ti.index_id
AND us.table_id = ti.descriptor_id
JOIN pg_catalog.pg_index AS pgidx ON indrelid = us.table_id
JOIN pg_catalog.pg_indexes AS pgidxs ON pgidxs.crdb_oid = indexrelid
AND indexname = ti.index_name
WHERE ti.descriptor_id = $::REGCLASS`,
tree.ConsoleLineWidth,
tree.PrettyAlignAndDeindent,
tree.UpperCase,
tableID,
)

Expand Down
38 changes: 11 additions & 27 deletions pkg/server/status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1949,33 +1949,17 @@ func TestStatusAPICombinedStatements(t *testing.T) {
thirdServerSQL := sqlutils.MakeSQLRunner(testCluster.ServerConn(2))

statements := []struct {
stmt string
formattedStmt string
fingerprint string
formattedFingerprint string
stmt string
fingerprinted string
}{
{stmt: `CREATE DATABASE roachblog`},
{stmt: `SET database = roachblog`},
{stmt: `CREATE TABLE posts (id INT8 PRIMARY KEY, body STRING)`},
{
stmt: `CREATE DATABASE roachblog`,
formattedStmt: "CREATE DATABASE roachblog\n",
},
{
stmt: `SET database = roachblog`,
formattedStmt: "SET database = roachblog\n",
},
{
stmt: `CREATE TABLE posts (id INT8 PRIMARY KEY, body STRING)`,
formattedStmt: "CREATE TABLE posts (id INT8 PRIMARY KEY, body STRING)\n",
},
{
stmt: `INSERT INTO posts VALUES (1, 'foo')`,
formattedStmt: "INSERT INTO posts VALUES (1, 'foo')\n",
fingerprint: `INSERT INTO posts VALUES (_, '_')`,
formattedFingerprint: "INSERT INTO posts VALUES (_, '_')\n",
},
{
stmt: `SELECT * FROM posts`,
formattedStmt: "SELECT * FROM posts\n",
stmt: `INSERT INTO posts VALUES (1, 'foo')`,
fingerprinted: `INSERT INTO posts VALUES (_, '_')`,
},
{stmt: `SELECT * FROM posts`},
}

for _, stmt := range statements {
Expand Down Expand Up @@ -2032,9 +2016,9 @@ func TestStatusAPICombinedStatements(t *testing.T) {

var expectedStatements []string
for _, stmt := range statements {
var expectedStmt = stmt.formattedStmt
if stmt.fingerprint != "" {
expectedStmt = stmt.formattedFingerprint
var expectedStmt = stmt.stmt
if stmt.fingerprinted != "" {
expectedStmt = stmt.fingerprinted
}
expectedStatements = append(expectedStatements, expectedStmt)
}
Expand Down

0 comments on commit c02b3b1

Please sign in to comment.