Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
88061: clisqlshell: new infrastructure for describe commands r=rafiss,ZhouXing19 a=knz

Fixes #95320.
Epic: CRDB-23454

The SQL shell (`cockroach sql`, `demo`) now
supports the client-side commands `\l`, `\dn`, `\d`, `\di`, `\dm`,
`\ds`, `\dt`, `\dv`, `\dC`, `\dT`, `\dd`, `\dg`, `\du`, `\df` and `\dd` in a
way similar to `psql`, including the modifier flags `S` and `+`, for
convenience for users migrating from PostgreSQL.

A notable difference is that when a pattern argument is specified, it
should use the SQL "LIKE" syntax (with `%` representing the wildcard
character) instead of PostgreSQL's glob-like syntax (with `*`
representing wildcards).

Issues discovered:

- [x] join bug:  #88096
- [x] semi-join exec error #91012
- [x] `pg_table_is_visible` should return true when given a valid index OID and the index is valid.  #88097
- [x] missing pkey column in pg_index:  #88106
- [x] missing stored columns in pg_index: #88107 
- [x] pg_statistic_ext has problems #88108
- [x] missing view def on materialized views  #88109
- [x] missing schema comments: #88098
- [x] missing pronamespace for functions #94952
- [x] broken pg_function_is_visible for UDFs #94953
- [x] generated columns #92545
- [x] indnullsnotdistinct #92583
- [x] missing prokind #95288
- [x] missing function comments in obj_description #95292
- [x] planning regression #95633

96397: builtins: mark some pg_.* builtins as strict r=DrewKimball a=mgartner

Builtins defined using the UDF `Body` field will be wrapped in a `CASE`
expression if they are strict, i.e., `CalledOnNullInput=false`. When the
builtin is inlined, the `CASE` expression prevents decorrelation,
leaving a slow apply-join in the query plan. This caused a significant
regression of some ORM introspection queries.

Some of these builtins have filters that cause the SQL body to return no rows
if any of the arguments is NULL. In this case, the builtin will have the same
behavior whether or not it is defined as being strict. We can safely optimize
these builtins by setting `CalledOnNullInput=true`.

The following conditions are sufficient to prove that `CalledOnNullInput` can
be set for a builtin function with a SQL body:

  1. The WHERE clause of the SQL query *null-rejects* every argument of the
     builtin. Operators like `=` and `<` *null-reject* their operands because
     they filter rows for which an operand is NULL.

  2. The arguments are not used elsewhere in the query. This is not strictly
     necessary, but simplifies the proof because it ensures NULL arguments will
     not cause the builtin to error.

Examples of SQL statements that would allow `CalledOnNullInput` to be set:
```
SELECT * FROM tab WHERE $1=1 AND $2='two';

SELECT * FROM tab WHERE $1 > 0;
```

Fixes #96218
Fixes #95569

Epic: None

Release note: None


97585: cli: don't scope TLS client certs to a specific tenant by default r=stevendanna a=knz

Epic: CRDB-23559
Fixes: #97584

This commit changes the default for `--tenant-scope` from "only the system tenant" to "cert valid for all tenants".

Note that the scoping is generally useful for security, and it is used in CockroachCloud. However, CockroachCloud does not use our CLI code to generate certs and sets its cert tenant scopes on its own.

Given that our CLI code is provided for convenience and developer productivity, and we don't expect certs generated here to be used in multi-tenant deployments where tenants are adversarial to each other, defaulting to certs that are valid on every tenant is a good choice.

Release note: None


Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
  • Loading branch information
3 people committed Feb 24, 2023
4 parents e7c2286 + 0c81bd1 + 649f219 + c50bd93 commit ae2e8b8
Show file tree
Hide file tree
Showing 21 changed files with 4,460 additions and 550 deletions.
4 changes: 2 additions & 2 deletions pkg/bench/rttanalysis/testdata/benchmark_expectations
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ exp,benchmark
4,ORMQueries/django_column_introspection_1_table
4,ORMQueries/django_column_introspection_4_tables
4,ORMQueries/django_column_introspection_8_tables
3,ORMQueries/django_table_introspection_1_table
3,ORMQueries/django_table_introspection_8_tables
5,ORMQueries/django_table_introspection_1_table
5,ORMQueries/django_table_introspection_8_tables
0,ORMQueries/has_column_privilege_using_attnum
0,ORMQueries/has_column_privilege_using_column_name
0,ORMQueries/has_schema_privilege
Expand Down
6 changes: 3 additions & 3 deletions pkg/cli/cliflags/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -793,9 +793,9 @@ Note: that --external-io-disable-http or --external-io-disable-implicit-credenti
TenantScope = FlagInfo{
Name: "tenant-scope",
Description: `Assign a tenant scope to the certificate.
This will allow for the certificate to only be used specifically for a particular
tenant. This flag is optional, when omitted, the certificate is scoped to the
system tenant.`,
This will restrict the certificate to only be valid for the specified tenants.
This flag is optional. When omitted, the certificate is not scoped; i.e.
it can be used with all tenants.`,
}

GeneratePKCS8Key = FlagInfo{
Expand Down
2 changes: 2 additions & 0 deletions pkg/cli/clisqlshell/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ go_library(
"api.go",
"complete.go",
"context.go",
"describe.go",
"doc.go",
"editor.go",
"editor_bimodal.go",
Expand Down Expand Up @@ -51,6 +52,7 @@ go_test(
name = "clisqlshell_test",
srcs = [
"complete_test.go",
"describe_test.go",
"editor_bubbline_test.go",
"main_test.go",
"sql_internal_test.go",
Expand Down
Loading

0 comments on commit ae2e8b8

Please sign in to comment.