opt: optimizer needs to be cancelable #70314
Labels
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
T-sql-queries
SQL Queries Team
Projects
Comments
cucaroach
added
C-bug
craig bot
pushed a commit
that referenced
this issue
Jul 27, 2022
84931: externalconn,nodelocal: add `external` ExternalStorage provider r=adityamaru a=adityamaru In #84310 we added the ability to create an external connection to represent a `nodelocal` endpoint. This diff is the second piece of the puzzle that allows systems in CRDB to interact with the external connection object. We introduce an `external` URI scheme to the `ExternalStorage` registry. URIs with an `external` scheme are required to contain a host component referring to the unique name of an existing extenral connection object. Optionally, the URI can also contain a path component that will be appended to the endpoint that was specified at the time the external connection object was created. This is necessary for operations such as backup and restore that read/write to subdirectories in the endpoint inputted by the user. A nice UX win is the abilility to have a single external connection object for the base bucket, and then interact with all the subdirectories without having to create an object for each directory. In the future we may want to clamp down on this, and allow the user to specify which objects permit subdirectory access. The `external://<object-name>/<optional-path>` URI is parsed and the underlying object is fetched from the `system.external_connections` table. The resource represented by the object is then `Dial()`ed to return an `ExternalStorage` handle that can be used to read, write, list etc. With this change all bulk operations and cdc are able to use external connections to represent a `nodelocal` endpoint. For example, a backup can now be run as: ``` CREATE EXTERNAL CONNECTION foo AS 'nodelocal://1/foo'; BACKUP INTO 'external://foo'; RESTORE FROM LATEST IN 'external://foo'; ``` Gradually, we will add support for all other external storage endpoints as well. Note, we do not register limiter settings for the `external` provider `ExternalStorage` interface, nor do we wrap it with an ioRecorder. This is because the underlying resource of the external connection object will already have its registered limiters and recorder. Informs: #84753 Release note (sql change): Bulk operations and CDC will accept an `external` scheme URI that points to a previously created External Connection object, These operations can then interact with the underlying resource represented by the object as they did before. 85041: sql: better logging about the optimizer as well as some cancellation checks r=yuzefovich a=yuzefovich **sql: add logging about the optimizer into the trace** This commit derives a separate tracing span on the main query path that covers the whole optimizer as well as adds a few logging messages around the optbuilder and the optimization. It also audits some of the already present logs to be written down before performing possibly long operations. Release note: None **xform: check for context cancellation** reviously, the optimizer didn't check the context cancellation ever which could lead to surprising behavior - e.g. if the statement timeout occurs in the middle of the optimization, we would still finish the whole optimization phase only to realize that we were canceled at the execution time. This situation is now improved by periodically checking for the context cancellation every time a group is optimized by `optimizeGroup`. It's still possible for the optimizer to continue running long after cancellation if there is a long running procedure at a deeper level. For example, a long-running, individual invocation of an exploration rule will not halt due to context cancellation. However, the cancellation improvement in this commit should halt long-running optimization in most cases. Fixes: #70245. Addresses: #70314. Release note: None 85101: kvcoord: remove an artifact of unsetting WriteTooOld flag r=yuzefovich a=yuzefovich It seems like during 20.1 release cycle we had a change so that the errors don't carry `WriteTooOld` flag set to `true` and now only the BatchResponse can have that. We kept the ability of unsetting that flag so that in a mixed-version scenario we wouldn't run into any issues, but now that unsetting behavior is no longer needed and is removed in this commit. My particular interest in removing this is because it is the only modification of the `SetupFlowRequest` proto (which carries the `Txn` proto inside) that occurs during the distributed query setup, and I want to have more parallelization there. Release note: None 85118: roachpb: remove `RangeDescriptor.deprecated_generation_comparable` r=pavelkalinnikov,andreimatei a=erikgrinaker This patch removes the unused field `deprecated_generation_comparable` from `RangeDescriptor`. The preparatory work for this was done in 20.2, see a578719. Release note: None Co-authored-by: Aditya Maru <adityamaru@gmail.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com>
|
#85041 addressed this partially, but it's still possible for long-running, lower-level logic to run without being canceled, like join reordering. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently the optimizer takes as long as it takes and there's limit to exploration. Lots of effort has been put into making it not take forever (limiting join reordering, the SkipReorderJoins flag) but some cases still take inordinate amounts of time (#70245). The optimizer needs to respect the statement timeout. Even if there isn't one it should have a built in reasonable default and return a best guess plan in that time frame. Maybe we could have an "optimizer_timeout" session variable, 0 meant go forever and default it to 1s. The rationale is that we'll never be able to be sure that the optimizer finishes in a bounded amount of time regardless of how many safeguards we put in place. If we can't produce a plan before the timeout we should probably also log a warning and maybe also increment a counter.
Jira issue: CRDB-10029
The text was updated successfully, but these errors were encountered: