Skip to content

Commit

Permalink
sql: hide number of placeholder
Browse files Browse the repository at this point in the history
Previously, placeholder values were kept as is
for fingerprint creation, meaning `IN ($1, $2)`
would be a different fingerprint than `IN ($2, $1)`
even though they should be the same.
This commit replace all placeholder values with `$1`, so we
can still know it was a placeholder, but we don't care about the
number of the placeholder itself.
This is the simplest solution to solve this problem. Other
options (e.g. replace with `$_` or `p_`) were considered but
ultimately they would require other modification on the parser code
to accept these values.

Previously:
`SELECT * FROM t WHERE v IN ($1, $2)` -> `SELECT * FROM t WHERE v IN ($1, $2)`
`SELECT * FROM t WHERE v IN ($2, $1)` -> `SELECT * FROM t WHERE v IN ($2, $1)`

Now:
`SELECT * FROM t WHERE v IN ($1, $2)` -> `SELECT * FROM t WHERE v IN ($1, $1)`
`SELECT * FROM t WHERE v IN ($2, $1)` -> `SELECT * FROM t WHERE v IN ($1, $1)`

Fixes #88074

Release note (sql change): The index of a placeholder
is now replaced to always be `$1` to limit fingerprint creations.
  • Loading branch information
maryliag committed Sep 20, 2022
1 parent 779f846 commit 6fa8bb4
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 36 deletions.
58 changes: 29 additions & 29 deletions pkg/sql/parser/testdata/backup_restore
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ BACKUP TABLE foo INTO $1 IN $2
----
BACKUP TABLE foo INTO $1 IN $2
BACKUP TABLE (foo) INTO ($1) IN ($2) -- fully parenthesized
BACKUP TABLE foo INTO $1 IN $2 -- literals removed
BACKUP TABLE foo INTO $1 IN $1 -- literals removed
BACKUP TABLE _ INTO $1 IN $2 -- identifiers removed

parse
Expand Down Expand Up @@ -171,7 +171,7 @@ SHOW BACKUP FROM $1 IN $2 WITH foo = 'bar'
----
SHOW BACKUP FROM $1 IN $2 WITH foo = 'bar'
SHOW BACKUP FROM ($1) IN ($2) WITH foo = ('bar') -- fully parenthesized
SHOW BACKUP FROM $1 IN $2 WITH foo = '_' -- literals removed
SHOW BACKUP FROM $1 IN $1 WITH foo = '_' -- literals removed
SHOW BACKUP FROM $1 IN $2 WITH _ = 'bar' -- identifiers removed

parse
Expand Down Expand Up @@ -203,7 +203,7 @@ SHOW BACKUP $1 IN $2 WITH foo = 'bar'
----
SHOW BACKUP $1 IN $2 WITH foo = 'bar'
SHOW BACKUP ($1) IN ($2) WITH foo = ('bar') -- fully parenthesized
SHOW BACKUP $1 IN $2 WITH foo = '_' -- literals removed
SHOW BACKUP $1 IN $1 WITH foo = '_' -- literals removed
SHOW BACKUP $1 IN $2 WITH _ = 'bar' -- identifiers removed

parse
Expand All @@ -227,15 +227,15 @@ BACKUP TABLE foo TO $1 INCREMENTAL FROM 'bar', $2, 'baz'
----
BACKUP TABLE foo TO $1 INCREMENTAL FROM 'bar', $2, 'baz'
BACKUP TABLE (foo) TO ($1) INCREMENTAL FROM ('bar'), ($2), ('baz') -- fully parenthesized
BACKUP TABLE foo TO $1 INCREMENTAL FROM '_', $2, '_' -- literals removed
BACKUP TABLE foo TO $1 INCREMENTAL FROM '_', $1, '_' -- literals removed
BACKUP TABLE _ TO $1 INCREMENTAL FROM 'bar', $2, 'baz' -- identifiers removed

parse
BACKUP foo TO $1 INCREMENTAL FROM 'bar', $2, 'baz'
----
BACKUP TABLE foo TO $1 INCREMENTAL FROM 'bar', $2, 'baz' -- normalized!
BACKUP TABLE (foo) TO ($1) INCREMENTAL FROM ('bar'), ($2), ('baz') -- fully parenthesized
BACKUP TABLE foo TO $1 INCREMENTAL FROM '_', $2, '_' -- literals removed
BACKUP TABLE foo TO $1 INCREMENTAL FROM '_', $1, '_' -- literals removed
BACKUP TABLE _ TO $1 INCREMENTAL FROM 'bar', $2, 'baz' -- identifiers removed

parse
Expand Down Expand Up @@ -301,15 +301,15 @@ BACKUP DATABASE foo TO ($1, $2)
----
BACKUP DATABASE foo TO ($1, $2)
BACKUP DATABASE foo TO (($1), ($2)) -- fully parenthesized
BACKUP DATABASE foo TO ($1, $2) -- literals removed
BACKUP DATABASE foo TO ($1, $1) -- literals removed
BACKUP DATABASE _ TO ($1, $2) -- identifiers removed

parse
BACKUP DATABASE foo TO ($1, $2) INCREMENTAL FROM 'baz'
----
BACKUP DATABASE foo TO ($1, $2) INCREMENTAL FROM 'baz'
BACKUP DATABASE foo TO (($1), ($2)) INCREMENTAL FROM ('baz') -- fully parenthesized
BACKUP DATABASE foo TO ($1, $2) INCREMENTAL FROM '_' -- literals removed
BACKUP DATABASE foo TO ($1, $1) INCREMENTAL FROM '_' -- literals removed
BACKUP DATABASE _ TO ($1, $2) INCREMENTAL FROM 'baz' -- identifiers removed

parse
Expand Down Expand Up @@ -400,39 +400,39 @@ RESTORE TABLE foo FROM $2 IN $1
----
RESTORE TABLE foo FROM $2 IN $1
RESTORE TABLE (foo) FROM ($2) IN ($1) -- fully parenthesized
RESTORE TABLE foo FROM $2 IN $1 -- literals removed
RESTORE TABLE foo FROM $1 IN $1 -- literals removed
RESTORE TABLE _ FROM $2 IN $1 -- identifiers removed

parse
RESTORE TABLE foo FROM $1, $2, 'bar'
----
RESTORE TABLE foo FROM $1, $2, 'bar'
RESTORE TABLE (foo) FROM ($1), ($2), ('bar') -- fully parenthesized
RESTORE TABLE foo FROM $1, $2, '_' -- literals removed
RESTORE TABLE foo FROM $1, $1, '_' -- literals removed
RESTORE TABLE _ FROM $1, $2, 'bar' -- identifiers removed

parse
RESTORE foo FROM $1, $2, 'bar'
----
RESTORE TABLE foo FROM $1, $2, 'bar' -- normalized!
RESTORE TABLE (foo) FROM ($1), ($2), ('bar') -- fully parenthesized
RESTORE TABLE foo FROM $1, $2, '_' -- literals removed
RESTORE TABLE foo FROM $1, $1, '_' -- literals removed
RESTORE TABLE _ FROM $1, $2, 'bar' -- identifiers removed

parse
RESTORE TABLE foo FROM 'abc' IN $1, $2, 'bar'
----
RESTORE TABLE foo FROM 'abc' IN $1, $2, 'bar'
RESTORE TABLE (foo) FROM ('abc') IN ($1), ($2), ('bar') -- fully parenthesized
RESTORE TABLE foo FROM '_' IN $1, $2, '_' -- literals removed
RESTORE TABLE foo FROM '_' IN $1, $1, '_' -- literals removed
RESTORE TABLE _ FROM 'abc' IN $1, $2, 'bar' -- identifiers removed

parse
RESTORE TABLE foo FROM $4 IN $1, $2, 'bar'
----
RESTORE TABLE foo FROM $4 IN $1, $2, 'bar'
RESTORE TABLE (foo) FROM ($4) IN ($1), ($2), ('bar') -- fully parenthesized
RESTORE TABLE foo FROM $4 IN $1, $2, '_' -- literals removed
RESTORE TABLE foo FROM $1 IN $1, $1, '_' -- literals removed
RESTORE TABLE _ FROM $4 IN $1, $2, 'bar' -- identifiers removed

parse
Expand Down Expand Up @@ -539,119 +539,119 @@ RESTORE DATABASE foo FROM ($1, $2)
----
RESTORE DATABASE foo FROM ($1, $2)
RESTORE DATABASE foo FROM (($1), ($2)) -- fully parenthesized
RESTORE DATABASE foo FROM ($1, $2) -- literals removed
RESTORE DATABASE foo FROM ($1, $1) -- literals removed
RESTORE DATABASE _ FROM ($1, $2) -- identifiers removed

parse
RESTORE DATABASE foo FROM ($1), ($2)
----
RESTORE DATABASE foo FROM $1, $2 -- normalized!
RESTORE DATABASE foo FROM ($1), ($2) -- fully parenthesized
RESTORE DATABASE foo FROM $1, $2 -- literals removed
RESTORE DATABASE foo FROM $1, $1 -- literals removed
RESTORE DATABASE _ FROM $1, $2 -- identifiers removed

parse
RESTORE DATABASE foo FROM ($1), ($2, $3)
----
RESTORE DATABASE foo FROM $1, ($2, $3) -- normalized!
RESTORE DATABASE foo FROM ($1), (($2), ($3)) -- fully parenthesized
RESTORE DATABASE foo FROM $1, ($2, $3) -- literals removed
RESTORE DATABASE foo FROM $1, ($1, $1) -- literals removed
RESTORE DATABASE _ FROM $1, ($2, $3) -- identifiers removed

parse
RESTORE DATABASE foo FROM ($1, $2), $3
----
RESTORE DATABASE foo FROM ($1, $2), $3
RESTORE DATABASE foo FROM (($1), ($2)), ($3) -- fully parenthesized
RESTORE DATABASE foo FROM ($1, $2), $3 -- literals removed
RESTORE DATABASE foo FROM ($1, $1), $1 -- literals removed
RESTORE DATABASE _ FROM ($1, $2), $3 -- identifiers removed

parse
RESTORE DATABASE foo FROM $1, ($2, $3)
----
RESTORE DATABASE foo FROM $1, ($2, $3)
RESTORE DATABASE foo FROM ($1), (($2), ($3)) -- fully parenthesized
RESTORE DATABASE foo FROM $1, ($2, $3) -- literals removed
RESTORE DATABASE foo FROM $1, ($1, $1) -- literals removed
RESTORE DATABASE _ FROM $1, ($2, $3) -- identifiers removed

parse
RESTORE DATABASE foo FROM ($1, $2), ($3, $4)
----
RESTORE DATABASE foo FROM ($1, $2), ($3, $4)
RESTORE DATABASE foo FROM (($1), ($2)), (($3), ($4)) -- fully parenthesized
RESTORE DATABASE foo FROM ($1, $2), ($3, $4) -- literals removed
RESTORE DATABASE foo FROM ($1, $1), ($1, $1) -- literals removed
RESTORE DATABASE _ FROM ($1, $2), ($3, $4) -- identifiers removed

parse
RESTORE DATABASE foo FROM ($1, $2), ($3, $4) AS OF SYSTEM TIME '1'
----
RESTORE DATABASE foo FROM ($1, $2), ($3, $4) AS OF SYSTEM TIME '1'
RESTORE DATABASE foo FROM (($1), ($2)), (($3), ($4)) AS OF SYSTEM TIME ('1') -- fully parenthesized
RESTORE DATABASE foo FROM ($1, $2), ($3, $4) AS OF SYSTEM TIME '_' -- literals removed
RESTORE DATABASE foo FROM ($1, $1), ($1, $1) AS OF SYSTEM TIME '_' -- literals removed
RESTORE DATABASE _ FROM ($1, $2), ($3, $4) AS OF SYSTEM TIME '1' -- identifiers removed

parse
RESTORE FROM ($1, $2)
----
RESTORE FROM ($1, $2)
RESTORE FROM (($1), ($2)) -- fully parenthesized
RESTORE FROM ($1, $2) -- literals removed
RESTORE FROM ($1, $1) -- literals removed
RESTORE FROM ($1, $2) -- identifiers removed

parse
RESTORE FROM ($1, $2), $3
----
RESTORE FROM ($1, $2), $3
RESTORE FROM (($1), ($2)), ($3) -- fully parenthesized
RESTORE FROM ($1, $2), $3 -- literals removed
RESTORE FROM ($1, $1), $1 -- literals removed
RESTORE FROM ($1, $2), $3 -- identifiers removed

parse
RESTORE FROM $1, ($2, $3)
----
RESTORE FROM $1, ($2, $3)
RESTORE FROM ($1), (($2), ($3)) -- fully parenthesized
RESTORE FROM $1, ($2, $3) -- literals removed
RESTORE FROM $1, ($1, $1) -- literals removed
RESTORE FROM $1, ($2, $3) -- identifiers removed

parse
RESTORE FROM ($1, $2), ($3, $4)
----
RESTORE FROM ($1, $2), ($3, $4)
RESTORE FROM (($1), ($2)), (($3), ($4)) -- fully parenthesized
RESTORE FROM ($1, $2), ($3, $4) -- literals removed
RESTORE FROM ($1, $1), ($1, $1) -- literals removed
RESTORE FROM ($1, $2), ($3, $4) -- identifiers removed

parse
RESTORE FROM ($1, $2), ($3, $4) AS OF SYSTEM TIME '1'
----
RESTORE FROM ($1, $2), ($3, $4) AS OF SYSTEM TIME '1'
RESTORE FROM (($1), ($2)), (($3), ($4)) AS OF SYSTEM TIME ('1') -- fully parenthesized
RESTORE FROM ($1, $2), ($3, $4) AS OF SYSTEM TIME '_' -- literals removed
RESTORE FROM ($1, $1), ($1, $1) AS OF SYSTEM TIME '_' -- literals removed
RESTORE FROM ($1, $2), ($3, $4) AS OF SYSTEM TIME '1' -- identifiers removed

parse
RESTORE FROM $1, $2, 'bar'
----
RESTORE FROM $1, $2, 'bar'
RESTORE FROM ($1), ($2), ('bar') -- fully parenthesized
RESTORE FROM $1, $2, '_' -- literals removed
RESTORE FROM $1, $1, '_' -- literals removed
RESTORE FROM $1, $2, 'bar' -- identifiers removed

parse
RESTORE FROM $4 IN $1, $2, 'bar'
----
RESTORE FROM $4 IN $1, $2, 'bar'
RESTORE FROM ($4) IN ($1), ($2), ('bar') -- fully parenthesized
RESTORE FROM $4 IN $1, $2, '_' -- literals removed
RESTORE FROM $1 IN $1, $1, '_' -- literals removed
RESTORE FROM $4 IN $1, $2, 'bar' -- identifiers removed

parse
RESTORE FROM $4 IN $1, $2, 'bar' AS OF SYSTEM TIME '1' WITH skip_missing_foreign_keys
----
RESTORE FROM $4 IN $1, $2, 'bar' AS OF SYSTEM TIME '1' WITH skip_missing_foreign_keys
RESTORE FROM ($4) IN ($1), ($2), ('bar') AS OF SYSTEM TIME ('1') WITH skip_missing_foreign_keys -- fully parenthesized
RESTORE FROM $4 IN $1, $2, '_' AS OF SYSTEM TIME '_' WITH skip_missing_foreign_keys -- literals removed
RESTORE FROM $1 IN $1, $1, '_' AS OF SYSTEM TIME '_' WITH skip_missing_foreign_keys -- literals removed
RESTORE FROM $4 IN $1, $2, 'bar' AS OF SYSTEM TIME '1' WITH skip_missing_foreign_keys -- identifiers removed

parse
Expand Down Expand Up @@ -694,15 +694,15 @@ RESTORE TENANT 36 FROM ($1, $2) AS OF SYSTEM TIME '1'
----
RESTORE TENANT 36 FROM ($1, $2) AS OF SYSTEM TIME '1'
RESTORE TENANT 36 FROM (($1), ($2)) AS OF SYSTEM TIME ('1') -- fully parenthesized
RESTORE TENANT _ FROM ($1, $2) AS OF SYSTEM TIME '_' -- literals removed
RESTORE TENANT _ FROM ($1, $1) AS OF SYSTEM TIME '_' -- literals removed
RESTORE TENANT 36 FROM ($1, $2) AS OF SYSTEM TIME '1' -- identifiers removed

parse
RESTORE TENANT 36 FROM ($1, $2) WITH tenant = '5'
----
RESTORE TENANT 36 FROM ($1, $2) WITH tenant = '5'
RESTORE TENANT 36 FROM (($1), ($2)) WITH tenant = ('5') -- fully parenthesized
RESTORE TENANT _ FROM ($1, $2) WITH tenant = '_' -- literals removed
RESTORE TENANT _ FROM ($1, $1) WITH tenant = '_' -- literals removed
RESTORE TENANT 36 FROM ($1, $2) WITH tenant = '5' -- identifiers removed

parse
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/parser/testdata/prepared_stmts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ PREPARE a (STRING, STRING) AS SELECT $1, $2
----
PREPARE a (STRING, STRING) AS SELECT $1, $2
PREPARE a (STRING, STRING) AS SELECT ($1), ($2) -- fully parenthesized
PREPARE a (STRING, STRING) AS SELECT $1, $2 -- literals removed
PREPARE a (STRING, STRING) AS SELECT $1, $1 -- literals removed
PREPARE _ (STRING, STRING) AS SELECT $1, $2 -- identifiers removed

parse
Expand Down Expand Up @@ -251,7 +251,7 @@ PREPARE a (STRING, STRING, STRING) AS IMPORT INTO a CSV DATA ($2) WITH temp = $3
----
PREPARE a (STRING, STRING, STRING) AS IMPORT INTO a CSV DATA ($2) WITH temp = $3
PREPARE a (STRING, STRING, STRING) AS IMPORT INTO a CSV DATA (($2)) WITH temp = ($3) -- fully parenthesized
PREPARE a (STRING, STRING, STRING) AS IMPORT INTO a CSV DATA ($2) WITH temp = $3 -- literals removed
PREPARE a (STRING, STRING, STRING) AS IMPORT INTO a CSV DATA ($1) WITH temp = $1 -- literals removed
PREPARE _ (STRING, STRING, STRING) AS IMPORT INTO _ CSV DATA ($2) WITH _ = $3 -- identifiers removed

parse
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/parser/testdata/select_clauses
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ SELECT $1, $2 FROM t
----
SELECT $1, $2 FROM t
SELECT ($1), ($2) FROM t -- fully parenthesized
SELECT $1, $2 FROM t -- literals removed
SELECT $1, $1 FROM t -- literals removed
SELECT $1, $2 FROM _ -- identifiers removed

parse
Expand Down Expand Up @@ -2801,7 +2801,7 @@ SELECT a FROM t FETCH FIRST $1 ROWS ONLY OFFSET $2 ROWS
----
SELECT a FROM t LIMIT $1 OFFSET $2 -- normalized!
SELECT (a) FROM t LIMIT ($1) OFFSET ($2) -- fully parenthesized
SELECT a FROM t LIMIT $1 OFFSET $2 -- literals removed
SELECT a FROM t LIMIT $1 OFFSET $1 -- literals removed
SELECT _ FROM _ LIMIT $1 OFFSET $2 -- identifiers removed

parse
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/sem/tree/format_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ func TestFormatNodeSummary(t *testing.T) {
},
{
stmt: `UPDATE system.jobs SET status = $2, payload = $3, last_run = $4, num_runs = $5 WHERE internal_table_id = $1`,
expected: `UPDATE system.jobs SET status = $2, pa... WHERE internal_table_...`,
expected: `UPDATE system.jobs SET status = $1, pa... WHERE internal_table_...`,
},
{
stmt: `UPDATE system.extra_extra_long_table_name SET (schedule_state, next_run) = ($1, $2) WHERE schedule_id = 'name'`,
Expand Down
5 changes: 4 additions & 1 deletion pkg/sql/sem/tree/hide_constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ func (ctx *FmtCtx) formatNodeOrHideConstants(n NodeFormatter) {
return
case *Placeholder:
// Placeholders should be printed as placeholder markers.
// Deliberately empty so we format as normal.
// Using always '$1' so we limit the amount of different
// fingerprints created.
ctx.WriteString("$1")
return
case *StrVal:
ctx.WriteString("'_'")
return
Expand Down
53 changes: 52 additions & 1 deletion pkg/sql/sqlstats/sslocal/sql_stats_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -740,12 +740,12 @@ func TestFingerprintCreation(t *testing.T) {
}()

testConn := sqlutils.MakeSQLRunner(sqlConn)
testConn.Exec(t, "SET application_name = 'app1'")
testConn.Exec(t, "CREATE TABLE t (v INT)")

var count int64

t.Run("test hide constants", func(t *testing.T) {
testConn.Exec(t, "SET application_name = 'app1'")
testCases := []struct {
stmt string
fingerprint string
Expand Down Expand Up @@ -1029,4 +1029,55 @@ func TestFingerprintCreation(t *testing.T) {
require.Equal(t, tc.count, count)
}
})

t.Run("test hide placeholders", func(t *testing.T) {
testConn.Exec(t, "SET application_name = 'app2'")
argsOne := []interface{}{1}
argsTwo := []interface{}{1, 2}
testCases := []struct {
stmt string
fingerprint string
count int64
literals []interface{}
}{
{
stmt: "SELECT * FROM t WHERE v IN ($1)",
fingerprint: "SELECT * FROM t WHERE v IN ($1,)",
count: 1,
literals: argsOne,
},
{
stmt: "SELECT * FROM t WHERE v IN (1)",
fingerprint: "SELECT * FROM t WHERE v IN (_,)",
count: 1,
},
{
stmt: "SELECT * FROM t WHERE v IN ($1, $2)",
fingerprint: "SELECT * FROM t WHERE v IN ($1, $1)",
count: 1,
literals: argsTwo,
},
{
stmt: "SELECT * FROM t WHERE v IN ($2, $1)",
fingerprint: "SELECT * FROM t WHERE v IN ($1, $1)",
count: 2,
literals: argsTwo,
},
{
stmt: "SELECT * FROM t WHERE v IN (1,2)",
fingerprint: "SELECT * FROM t WHERE v IN (_, _)",
count: 1,
},
}

for _, tc := range testCases {
testConn.Exec(t, tc.stmt, tc.literals...)
rows := testConn.QueryRow(t, "SELECT statistics -> 'statistics' ->> 'cnt' FROM "+
"CRDB_INTERNAL.STATEMENT_STATISTICS WHERE app_name = 'app2' "+
"AND metadata ->> 'query'=$1", tc.fingerprint)
rows.Scan(&count)

require.Equal(t, tc.count, count)
}
})
}

0 comments on commit 6fa8bb4

Please sign in to comment.