cli: refactors; make cockroach-sql's CLI parameter handling match cockroach sql
#82020
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
knz
force-pushed
the
20220528-certs
branch
2 times, most recently
from
45e9e96 to
dfc8c86
Compare
knz
changed the title
knz
changed the title
cockroach-sql's CLI parameter handling match cockroach sql
|
NB: Reviewable seems to have issues displaying the entire commit sequence. The Github review interface is fine with it. If the reviewer(s) would prefer that, I can split this PR into a sequence of small(er) PRs. |
knz
changed the title
cockroach-sql's CLI parameter handling match cockroach sqlcockroach-sql's CLI parameter handling match cockroach sql
otan
reviewed
May 29, 2022
There was a problem hiding this comment.
mostly some naming nits here, curious about how you feel about them
the clientconnurl package and clienturl package is a little similarly named and easy to get confused by. what are your thoughts on:
- clientconnurl -> pgconnurl (is it client specific?)
- clienturl -> cliurl, to make it clear only the CLI should be using it?
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| package certnames |
There was a problem hiding this comment.
optional nit: docstring would be nice. should we use the singular form certname - seems like what most packages are?
feel free to tack this on as the last commit, i'm not fussed if you choose not to action this
There was a problem hiding this comment.
added docstring and chose better names
| ) | ||
|
|
||
| // A CertsLocator provides locations to certificates. | ||
| type CertsLocator struct { |
There was a problem hiding this comment.
another optional nit: do you think "Locator" is a good name for this, slightly removes some stuttering?
There was a problem hiding this comment.
agreed. Done (in split PR)
| ) | ||
|
|
||
| // AssetLoader describes the functions necessary to read certificate and key files. | ||
| type AssetLoader struct { |
There was a problem hiding this comment.
optional nit: using Loader can reduce stuttering
pkg/cli/client_url.go
Outdated
| func updateURL( | ||
| newURL string, | ||
| sslStrict bool, | ||
| makeStrictErr func() error, |
There was a problem hiding this comment.
optional: how do you feel about these functions being an interface instead?
could add unit tests too, if we wanted to. having to pass a lot of hooks in is easy to lose track of.
There was a problem hiding this comment.
good idea. done (in split PR)
pkg/cmd/cockroach-sql/main.go
Outdated
| return err | ||
| } | ||
|
|
||
| // How we're going to authenticate. |
There was a problem hiding this comment.
nit: comment sounds like a question - maybe "Get how we're going to authenticate"
|
Yes I agree some names here are unfortunate. I will split this PR so we can iterate on the review more easily. |
This was referenced May 30, 2022
Merged
We want to have the cert/key filenames in a package with minimal dependencies, suitable for embedding in the lightweight SQL executable. This commit starts the work by moving the file name constants in a new separate package. Release note: None
This simplifies the code and makes it more readable. Release note: None
knz
force-pushed
the
20220528-certs
branch
2 times, most recently
from
00eaddb to
cc7bcff
Compare
This is to prepare its use in the cockroach-sql standalone program. Release note: None
This will be needed to ensure `cockroach-sql` supports the same env vars. Release note: None
This ensures that the basic primitives are reusable out of package `cli`. Release note: None
Release note: None
So that the caller can control at which level the env var consumer check occurs. (This can be explored via `cockroach debug env`.) Release note: None
Release note: None
Release note (cli change): The standalone `cockroach-sql` executable now has more compatibility with `cockroach sql`, so it can be used as a drop-in replacement. For example, it supports running without a URL, using connection defaults. It also supports overriding `--certs-dir` and other client-side options also supported by `cockroach sql`.
|
All the split-out PRs have been individually approved. Moving forward here. bors r=otan,catj-cockroach |
|
Build failed: |
|
unrelated flake bors r=otan,catj-cockroach |
Release note: None
|
Canceled. |
|
added a skip bors r=otan,catj-cockroach |
|
Build succeeded: |
rafiss
added a commit
to rafiss/cockroach
that referenced
this pull request
Sep 6, 2022
cockroachdb#82020 accidentally broke the logic for the certs-dir defaulting to ${HOME}/.cockroach-certs/ No release note since this was not released. Release note: None Release justification: low risk bug fix to new functionality.
rafiss
added a commit
to rafiss/cockroach
that referenced
this pull request
Sep 7, 2022
cockroachdb#82020 accidentally broke the logic for the certs-dir defaulting to ${HOME}/.cockroach-certs/ No release note since this was not released. Release note: None Release justification: low risk bug fix to new functionality.
craig bot
pushed a commit
that referenced
this pull request
Sep 7, 2022
87174: sql: index rec with NotVisible indexes r=mgartner,maryliag a=wenyihu6 **sql: introduce index rec type** This commit introduces a type indexRecType which acts as an enum to represent the type of index recommendation. This commit does not change any existing functionality, and the main purpose is to make future commits cleaner. Release justification: This is a low risk change to the existing functionality. Release note: None --- **sql: finds better index to replace in index rec** Background information on how index recommendation works now: It first finds possible index candidates with the given query. These index candidates determine which columns to store inside hypothetical indexes definition. In addition, the hypothetical indexes store every other column from the table (as they are stored in a STORING clause). Then, the index recommender calls the optimizer with the updated table that stores both existing and hypothetical indexes. The index recommender then scans over the optimal plan and sees if a hypothetical index is used. If so, an index recommendation is constructed. However, we do not know the entire set of useful columns yet. While processing the entire optimal plan, we update the index recommendation instances to add useful columns. Prior to this commit, index recommendation decides what existing index can be potentially replaced when the index recommendation is constructed. It looks for the first existing index with the same explicit column as the hypothetical index definition. This is not ideal because the best existing index to replace might not be the first one. This commit changes the logic so that the index recommender decides what the best existing index to replace is after it fully processes the optimal plan and decides the entire set of columns that are useful. The best existing index to replace is the one that stores the same explicit column as the hypothetical index and stores the most columns from what was actually scanned. Consider the following example. ``` CREATE INDEX idx_1 ON t(v) CREATE INDEX idx_2 ON t(v) STORING (i) ``` Given `SELECT i, j FROM t5 WHERE v > 1`, the index recommendation previously recommends replacing idx_1 with `idx(v) STORING (i, j)`. It now recommends replacing `idx_2 with idx(v) STORING (i, j)` as idx_2 is more similar to what we want to replace later on. In addition, this commit also makes adding invisible indexes to index recommender much easier. Release justification: This is a low risk change to the existing functionality. Release note: Instead of always recommending to replace the first existing index with the same explicit columns, the index recommendation now considers all existing indexes in the table to decide the best one to replace. --- **sql: index rec with NotVisible indexes** Now that we have introduced not visible index feature, we should update index recommendations accordingly. Prior to this commit, the index recommendation engine treats all indexes as they are visible. If an index already exists in the table and is the same as we want to recommend, this index recommendation is not created. However, it is possible that this index is not visible now. This commits fixes this by adding not visible indexes to the logic. The index recommender only recommends alter index visible if the index already contains every explicit column in the hypothetical index and every useful column. It will never recommend dropping an invisible index. Fixes: #85477 Release justification: This commit does not interfere with existing functionality and fixes the index recommendation engine to take an index visibility into account. Release note: Index recommendation now considers not visible indexes and can also make index recommendations for alter index … visible. 87318: builtins: hide crdb_internal.probe_ranges from docs r=rafiss a=rafiss fixes #84758 Release note: None Release justification: docs only change 87403: storage: tighten `MVCCDeleteRangeUsingTombstone` key span checks r=sumeerbhola a=erikgrinaker **storage: tighten `MVCCDeleteRangeUsingTombstone` key span checks** We've previously disallowed writing MVCC range tombstones across local keyspace. However, this check used `keys.IsLocal()`, which simply checks whether the key has the local prefix (`\x01`), so it was possible for callers to use e.g. `\x00` as a start key and span the local keyspace anyway. This patch changes the check to ensure the start key is at or after `keys.LocalMax`. Release justification: bug fixes and low-risk updates to new functionality Release note: None **storage: fix `intentInterleavingIter` local key detection** `intentInterleavingIter` will error if given bounds that span from the local to the global keyspace. However, it only checked whether the start key satisfied `keys.IsLocal()`, i.e. that the key began with `\x01`. This meant that it was possible to give a start key with a `\x00` prefix that spanned across the local and global keyspaces, without triggering the error. This could cause the iterator to be incorrectly positioned in the lock table keyspace. This patch fixes the check by comparing the start key with `keys.LocalMax` instead. This does not appear to have a measurable impact on performance: ``` name old time/op new time/op delta MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=0-24 2.39µs ± 1% 2.39µs ± 1% ~ (p=0.810 n=10+10) ``` Resolves #87489. Release justification: low risk, high benefit changes to existing functionality Release note: None 87443: sql/importer: add hints on null value import failure for CSV files r=rafiss,ZhouXing19 a=andyyang890 This patch adds hints notifying the user when an `IMPORT INTO ... CSV DATA` command fails due to null values in the CSV file having leading or trailing whitespace or being quoted when the `allow_quoted_null` option is not set. Fixes #83723 Release justification: low risk, high benefit changes to existing functionality Release note (sql change): A hint will now be provided when importing from a CSV file fails due to null values having leading or trailing whitespace or being quoted when the `allow_quoted_null` option is not set. 87450: cli: fix default certs-dir r=knz a=rafiss fixes #87191 #82020 accidentally broke the logic for the certs-dir defaulting to ${HOME}/.cockroach-certs/ No release note since this bug was not released. Release note: None Release justification: low risk bug fix to new functionality. 87461: add Alvin Bao to authors r=baoalvin1 a=baoalvin1 Release note: None Release justification: non-production code change. 87493: authors: add <[email protected]> to authors r=amyyq2 a=amyyq2 Release note: None Release justification: non-production code change 87504: sql: add schema_name to index usage stat telemetry, add descriptions to proto fields r=ericharmeling a=ericharmeling The first commit in this PR adds descriptions to the index usage telemetry proto fields. Fixes #85855. Release justification: non-production code changes Release note: None ---- The second commit in this PR adds the schema name to the index usage statistics telemetry. Fixes #85932. Release justification: low risk, high benefit changes to existing functionality Release note (sql change): Added the schema name to index usage statistics telemetry. Co-authored-by: wenyihu3 <[email protected]> Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: Erik Grinaker <[email protected]> Co-authored-by: Andy Yang <[email protected]> Co-authored-by: Alvin Bao <[email protected]> Co-authored-by: Amy Qian <[email protected]> Co-authored-by: Eric Harmeling <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note to the reviewers:
cliinto a sub-package ofsecuritywhere it belongs.cockroach sql, which at this point should be considered sufficient given we're using the same config code incockroach-sqlnow. To make tests work across the two commands, we would need to make progress on build: need a testing framework to exercise the produced cockroach-sql binary #80921 first.Fixes #81882.
Fixes #82024.
Unblocks progress on #29285.