Ajwerner/wrapped descriptors #6

ajwerner · 2020-06-08T14:33:53Z

No description provided.

A previous change (cockroachdb#49721) adding a benchmark for ExportToSst left in some extra unused variables in a helper method. This change cleans that up. Release note: None.

This commit extract most of the logic of `createTableReaders` into a separate method that performs the actual physical planning of table readers given already created `TableReaderSpec`s. This will allow for reuse in the follow-up work. This commit additionally slightly refactors a few other things for reuse. Release note: None

This commit extracts the logic of `execFactory.ConstructPlan` into the function to be used by both factories. It also extracts out another small helper. Release note: None

Also fixed a bug where `\0` didn't actually output a NULL terminator (no idea why it worked). Release note (sql change): Populate the spatial_ref_sys table with support SRID entries for geospatial data types.

This commit is automatic replacement of `apd.Decimal.SetFinite((.*), 0)` into a nicer `apd.Decimal.SetInt64($1)` which are equivalent. Release note: None

Trying to make the geo package not depend on GEOS, and this is one of the required steps. Release note: None

This was making it hard to run the same benchmarks in a consistent way. Release note: None

The .eg.go files had unnecessary imports of execgen, which is not required in the runtime generated code, and in fact is suspicious. Release note: None

This commit adds a new directive to execgen: // execgen:inline This directive causes a function that's annotated with it to be inlined into all of its callers via AST transformation. This kind of inlining is smarter than before - it permits arguments with names different from the names of the function's formal parameters, for example. This commit also changes the functions in distinct_tmpl to use this directive instead of the manual templating method. I think this is the only easy function to change. The rest have, at the simple level of difficulty, static template-time parameters, and at a harder level of difficulty, type parameters. Improving these cases holistically should come next. Release note: None

…e a sentinel ErrFileDoesNotExist error The `ReadFile` interface method is responsible for returning a `Reader` which can be used to stream data from an external storage source. There are also instances where we use this method to solely check for the existence (or absence) of a particular file/object, by attempting to open a stream. eg: checking for BackupManifest/BackupManifestCheckpoint before exporting data. Previously, we would treat any error returned by the storage vendor API as a signal for the file/object not existing. This change adds logic to catch the native "file does not exist" errors for each storage provider, and throw a sentinel error to users of the `ReadFile` method. This allows for more careful error handling. Relevant unit tests have also been added. Release note: None

There's at lease one PR with a missing branch tips in refs/pull. This commit makes the script tolerant of that failure. Release note: None

49852: storage: Remove extra vars from runExportToSst r=itsbilal a=itsbilal A previous change (cockroachdb#49721) adding a benchmark for ExportToSst left in some extra unused variables in a helper method. This change cleans that up. Release note: None. Co-authored-by: Bilal Akhtar <bilal@cockroachlabs.com>

Resume verb is already used to for the action of starting a job after adoption on a node. Here instead Resume is the opposite action of pausing a job. Release note: none.

Fixes cockroachdb#49809. This PR disallows using types from other databases in tables. This makes certain behavior (like `DROP TYPE`) more predictable in their effects, as well as unblocking some work for supporting user defined types in `cockroach dump`. Release note (sql change): Referencing types across databases has been disabled.

As docgen relies on builtins which relies on proj, we are stuck in a situation in cross compilation where we are trying to install docgen with a non-native libproj.a file causing the linker to fail in `Upload Binaries`. Fix this by not compiling docgen on cross compilations. Also fix the windows compilation as PROJ will dump it into a libproj_4_9.a instead of libproj.a. Cannot see an option to fix this on the CMakeLists.txt. Also make execgen not depend on LIBPROJ. Release note: None

This commit adds an implementation of `distSQLSpecExecFactory.ConstructScan` which combines the logic that performs `scanNode` creation of `execFactory.ConstructScan` and physical planning of table readers of `DistSQLPlanner.createTableReaders`. I tried to refactor the code so that there is not much duplication going on. Notably, simple projections, renders, and filters are not yet implemented. Release note: None

After the examining the code closer, I see that now in all code paths we populate `scanColumnsConfig.wantedColumns` to ask for the specific columns to be scanned. Previously, this map could be left as `nil` which would mean that all columns of the table should be scanned. This was used only by the heuristic planner which has been removed, so we can now update the assumption of the scan columns config. This allows us to clean up the comments a bit. This commit also moves `makeColumnsConfig` function into the shared util file because it is used by both factories. Release note: None

Prior to this commit, a recursive CTE in which the cardinality of the left side of the UNION ALL expression was zero would cause an error in the statistics code, "estimated row count must be non-zero". This was happening because the cardinality of the recursive CTE binding props was set to be non-zero, but the row count, which came from the left side expression, was not updated accordingly. The stats code only allows the row count to be zero if the cardinality is also zero. This commit fixes the problem by setting the row count of the binding props to 1 if the estimated row count of the left side is less than 1. Fixes cockroachdb#49911 Release note (bug fix): Fixed an internal planning error that occured for recursive CTEs (WITH RECURSIVE expressions) in which the left side of the UNION ALL query used in the CTE definition produced zero rows.

A message had an unescaped format and ended up rendered like: use YourFuncf("descriptive prefix %!s(MISSING)", ...) Release note: None

49913: cloud: Added support to all ExternalStorage ReadFile methods, to raise a sentinel ErrFileDoesNotExist error r=adityamaru a=adityamaru The `ReadFile` interface method is responsible for returning a `Reader` which can be used to stream data from an external storage source. There are also instances where we use this method to solely check for the existence (or absence) of a particular file/object, by attempting to open a stream. eg: checking for BackupManifest/BackupManifestCheckpoint before exporting data. Previously, we would treat any error returned by the storage vendor API as a signal for the file/object not existing. This change adds logic to catch the native "file does not exist" errors for each storage provider, and throw a sentinel error to users of the `ReadFile` method. This allows for more careful error handling. Relevant unit tests have also been added. Release note: None Co-authored-by: Aditya Maru <adityamaru@gmail.com>

Incorporate volatility of operators (unary, binary, comparison) into the VolatilitySet property. Unfortunately, this modifies most plans as pretty much everything is "immutable". Perhaps once this work is behind us we will want to hide this information from plans in most cases. Release note: None

…db#49958 cockroachdb#49961 49728: execgen: add generic inliner r=jordanlewis a=jordanlewis This commit adds a new directive to execgen: // execgen:inline This directive causes a function that's annotated with it to be inlined into all of its callers via AST transformation. This kind of inlining is smarter than before - it permits arguments with names different from the names of the function's formal parameters, for example. This commit also changes the functions in distinct_tmpl to use this directive instead of the manual templating method. I think this is the only easy function to change. The rest have, at the simple level of difficulty, static template-time parameters, and at a harder level of difficulty, type parameters. Improving these cases holistically should come next. Release note: None 49841: sql: disallow cross database type references r=otan a=rohany Fixes cockroachdb#49809. This PR disallows using types from other databases in tables. This makes certain behavior (like `DROP TYPE`) more predictable in their effects, as well as unblocking some work for supporting user defined types in `cockroach dump`. Release note (sql change): Referencing types across databases has been disabled. 49919: sql: use nicer apd.Decimal.SetInt64 in the code base r=yuzefovich a=yuzefovich This commit is automatic replacement of `apd.Decimal.SetFinite((.*), 0)` into a nicer `apd.Decimal.SetInt64($1)` which are equivalent. Release note: None 49958: jobs: rename Resume to Unpause r=spaskob a=spaskob Resume verb is already used to for the action of starting a job after adoption on a node. Here instead Resume is the opposite action of pausing a job. Release note: none. 49961: opt: fix error caused by recursive CTE with zero rows on left side r=rytaft a=rytaft Prior to this commit, a recursive CTE in which the cardinality of the left side of the `UNION ALL` expression was zero would cause an error in the statistics code, "estimated row count must be non-zero". This was happening because the cardinality of the recursive CTE binding props was set to be non-zero, but the row count, which came from the left side expression, was not updated accordingly. The stats code only allows the row count to be zero if the cardinality is also zero. This commit fixes the problem by setting the row count of the binding props to 1 if the estimated row count of the left side is less than 1. Fixes cockroachdb#49911 Release note (bug fix): Fixed an internal planning error that occured for recursive CTEs (WITH RECURSIVE expressions) in which the left side of the UNION ALL query used in the CTE definition produced zero rows. Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com> Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Spas Bojanov <spas@cockroachlabs.com> Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>

`sql/pgwire/testdata/encodings.json` is autogenerated using `cmd/generate-binary/main.go`. Previously, a few tests were missing from this file, which would have been lost the next time new tests were added and `encodings.json` was autogenerated. This PR fixes that by moving the tests to the auto-gen script. Release note (none)

49682: sql: implement ConstructScan in the new factory r=yuzefovich a=yuzefovich **sql: extract out planning of table readers into a separate method** This commit extract most of the logic of `createTableReaders` into a separate method that performs the actual physical planning of table readers given already created `TableReaderSpec`s. This will allow for reuse in the follow-up work. This commit additionally slightly refactors a few other things for reuse. Release note: None **sql: implement ConstructPlan by new factory** This commit extracts the logic of `execFactory.ConstructPlan` into the function to be used by both factories. It also extracts out another small helper. Release note: None **sql: implement ConstructScan in distsql spec factory** This commit adds an implementation of `distSQLSpecExecFactory.ConstructScan` which combines the logic that performs `scanNode` creation of `execFactory.ConstructScan` and physical planning of table readers of `DistSQLPlanner.createTableReaders`. I tried to refactor the code so that there is not much duplication going on. Notably, simple projections, renders, and filters are not yet implemented. Closes: cockroachdb#47474. Release note: None **sql: enforce the assumption that `wantedColumns` is not nil for scans** After the examining the code closer, I see that now in all code paths we populate `scanColumnsConfig.wantedColumns` to ask for the specific columns to be scanned. Previously, this map could be left as `nil` which would mean that all columns of the table should be scanned. This was used only by the heuristic planner which has been removed, so we can now update the assumption of the scan columns config. This allows us to clean up the comments a bit. This commit also moves `makeColumnsConfig` function into the shared util file because it is used by both factories. Release note: None Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>

49949: geo/geogfn: implement ST_Project r=otan a=hueypark Fixes cockroachdb#48402 Release note (sql change): This PR implement adds the ST_Project({geography,float8,float8}) Co-authored-by: Jaewan Park <jaewan.huey.park@gmail.com>

Fixes cockroachdb#29021 Only create view dependencies on columns if the column is referenced in the view query. Release note (sql change): Views now only create a dependency on a table's column if the column is referenced in the view definition. Previously, all columns were added as a dependency meaning if a table was referenced in a view, all columns regardless of if the column was actually referenced would be added to the view's dependencies.

Fixes cockroachdb#49977 Parallel importer could get stuck due to a race between emitted import batches and checking for context cancellation (either due to an unforeseen error, or due to explicit context cancallation). Fix the race condition, and add tests verifying correct behavior. Release notes (bug fix): correctly handle import cancellation and errors.

49872: sql: track fine-grained view dependencies r=rytaft a=RichardJCai Fixes cockroachdb#29021 Only create view dependencies on columns if the column is referenced in the view query. Note: this does not fix the dependencies of old views. Release note (sql change): Views now only create a dependency on a table's column if the column is referenced in the view definition. Previously, all columns were added as a dependency meaning if a table was referenced in a view, all columns regardless of if the column was actually referenced would be added to the view's dependencies. 49888: geo: fix GeoJSON and allow options in ST_AsGeoJSON r=sumeerbhola a=otan There was a bug in GeoJSON where we used the high level name:"Feature" field, as opposed to the inner Geometry. This has been fixed. We also support more options for ST_AsGeoJSON now that these changes are in twpayne/go-geom. Resolves cockroachdb#48381. Release note (sql change): Implement ST_AsGeoJSON with options to show bbox and CRS information. 49931: opt: add rule to fold limits r=DrewKimball a=DrewKimball Previously, there was no rule to fold a Limit on top of a Limit when the outer limit value is smaller than the inner limit value. This patch adds a rule to fold two Limits together when the outer Limit has a smaller limit value than the inner Limit and the inner ordering implies the outer ordering. Release note (sql change): The optimizer can now fold two Limit operators together. Co-authored-by: richardjcai <caioftherichard@gmail.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com> Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>

49979: importccl: Correctly handle errors and cancellations during import. r=miretskiy a=miretskiy Fixes cockroachdb#49977 Parallel importer could get stuck due to a race between emitted import batches and checking for context cancellation (either due to an unforeseen error, or due to explicit context cancallation). Fix the race condition, and add tests verifying correct behavior. Release notes (bug fix): correctly handle import cancellation and errors. Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>

Allow and block aren't hurtful terms, and they also happen to be clearer than white and black - they connote meaning instantly. c.f.: https://twitter.com/bradfitz/status/1269449722063773696?s=20 rails/rails#33677 https://gitlab.com/gitlab-org/gitlab/-/issues/7554 lagom/lagom#2532 https://bugs.chromium.org/p/chromium/issues/detail?id=981129 Release note: None

Release note (sql change): Introduce maxDecimalDigits arguments for ST_AsText and ST_AsEWKT, which allow rounding of the decimal digits output in the WKT representation.

49875: geo: handle maximum decimal digits for ST_AsText r=sumeerbhola a=otan Resolves cockroachdb#48387. Resolves cockroachdb#48885. Release note (sql change): Introduce maxDecimalDigits arguments for ST_AsText and ST_AsEWKT, which allow rounding of the decimal digits output in the WKT representation. Co-authored-by: Oliver Tan <otan@cockroachlabs.com>

49946: *: s/whitelist/allowlist/, s/blacklist/blocklist/ r=jordanlewis a=jordanlewis Allow and block aren't hurtful terms, and they also happen to be clearer than white and black - they connote meaning instantly. Even though blacklist and whitelist don't have racist intent, they nevertheless propagate an unconscious "white = allow", "black = deny" meaning. There's no need to keep these terms - they're just terms. This is just one tiny thing we can do to improve our industry's inclusivity. c.f.: https://twitter.com/bradfitz/status/1269449722063773696?s=20 rails/rails#33677 https://gitlab.com/gitlab-org/gitlab/-/issues/7554 lagom/lagom#2532 https://bugs.chromium.org/p/chromium/issues/detail?id=981129 Release note: None Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>

This change refactors following components to use CSS modules instead of styles defined as global: - Dropdown - Search - PageConfig Release note: None

48012: ui: CSS modules for Statements filter section r=koorosh a=koorosh Depends on cockroachdb#47606 Related to cockroachdb#47527 This change refactors following components to use CSS modules instead of styles defined as global: - Dropdown - Search - PageConfig Release note: None Co-authored-by: Andrii Vorobiov <and.vorobiov@gmail.com>

This commit modifies structured.proto to duplicate the fields required for leasing into each of the descriptor types. It does not yet adopt these fields. This commit is part of a much larger change to wrap descriptor proto structs used throughout the codebase. To contextualize the scope of this change, in an earlier iteration this commit, instead of duplicating these fields, opted to create a new message which was to be stored in the `Descriptor` type itself. While that change brought with it some niceties, it also brought the requirement to wrap the raw descriptor structs as they no longer carried all of their relevant metadata. This duplication approach in many ways obviates the pressing need for much of this PR but, nevertheless, we pursue it because it's part of a broader trend we'd like to enforce. In many ways the scope of this PR has become opportunistic rather than necessary. Release note: None

For now, move the Descriptor interface down into sqlbase as part of the effort to elimate sqlbase.DescriptorProto. Release note: None

In preparation for merging that interface with DescriptorInterface. Release note: None

This change is large but is largely mechanical. The basic idea is that we want to stop passing around raw pointers to protobuf structs. Instead we'd like to pass around higher-level wrappers which implement interfaces. There's some room for debate about the precise nature of Mutable/Immutable wrappers for descriptor types but over time, hopefully, we'll move descriptor manipulation behind clear interfaces. It's worth noting that this commit is a half-step. It began, somewhat unfortunately, by introducing the TypeDescriptor wrappers and then goes all the way to deleting the `DescriptorProto` interface and unexporting `WrapDescriptor`. Release note: None

…escriptor Next we'll stop having the raw descriptor implement that interface. Release note: None

This is a large but also mostly mechanical change. There's room for improvement on the uniformity of it all but let's leave that be for now as I'm getting tired and need to keep making progress. It'll be easier to clean it up later. In particular, I've increasingly been feeling that an interface-based approach to these descriptors would be worthwhile and that that would be easy to accomplish for database but alas. That will have to be another big and mechanical change. Release note: None

This commit has GetDescriptorByID return an "unwrapped" descriptor and removes `UnwrapDescriptor` function from sqlbase. This terminology is all crap given the `UnwrapDescriptor` function returned a descriptor that is itself sort of wrapped. Good news, this terminology is going away. There is a hodge-podge of other unfortunate touch up in this commit. Release note: None

Release note: None

This commit made a lot more sense when it removed those fields from the proto and lifted them to a new message. It's still not a terrible commit so we'll leave it. It should be handy when we transition descriptor interactions to be through interfaces. Release note: None

This commit, like others, is not nearly as urgent given we're keeping the meta fields on the database descriptor. This commit adopts the interface methods for accessing the ID and Name of a DatabaseDescriptor. The reworking in this commit of previous changes also in this PR is annoying, I'm sorry to the reviewers. I'm also increasingly sensing the urgency of eschewing the concrete descriptor wrappers in favor of interfaces but I'm not going to try to deal with that in this PR. Release note: None

In anticipation of lifting methods to the wrapper types. Release note: None

Release note: None

This commit makes more retrieval functions "unwrap" descriptors into DescriptorInterface before returning them. It somewhat unfortunately pushes that responsibility into the sql/resolver in order to accomodate backups. Release note: None

This commit is the ugly side of copying the leasing fields into each of the descriptors. This leaves the `Descriptor.Table` method in place for now as it forms defense in depth. We'll remove it eventually when this all gets cleaned up. Release note: None

…db#80762 cockroachdb#80773 79911: opt: refactor and test lookup join key column and expr generation r=mgartner a=mgartner #### opt: simplify fetching outer column in CustomFuncs.findComputedColJoinEquality Previously, `CustomFuncs.findComputedColJoinEquality` used `CustomFuncs.OuterCols` to retrieve the outer columns of computed column expressions. `CustomFuncs.OuterCols` returns the cached outer columns in the expression if it is a `memo.ScalarPropsExpr`, and falls back to calculating the outer columns with `memo.BuildSharedProps` otherwise. Computed column expressions are never `memo.ScalarPropsExpr`s, so we use just use `memo.BuildSharedProps` directly. Release note: None #### opt: make RemapCols a method on Factory instead of CustomFuncs Release note: None #### opt: use partial-index-reduced filters when building lookup expressions This commit makes a minor change to `generateLookupJoinsImpl`. Previously, equality filters were extracted from the original `ON` filters. Now they are extracted from filters that have been reduced by partial index implication. This has no effect on behavior because equality filters that reference columns in two tables cannot exist in partial index predicates, so they will never be eliminated during partial index implication. Release note: None #### opt: moves some lookup join generation logic to lookup join package This commit adds a new `lookupjoin` package. Logic for determining the key columns and lookup expressions for lookup joins has been moved to `lookupJoin.ConstraintBuilder`. The code was moved with as few changes as possible, and the behavior does not change in any way. This move will make it easier to test this code in isolation in the future, and allow for further refactoring. Release note: None #### opt: generalize lookupjoin.ConstraintBuilder API This commit makes the lookupjoin.ConstraintBuilder API more general to make unit testing easier in a future commit. Release note: None #### opt: add data-driven tests for lookupjoin.ConstraintBuilder Release note: None #### opt: add lookupjoin.Constraint struct The `lookupjoin.Constraint` struct has been added to encapsulate multiple data structures that represent a strategy for constraining a lookup join. Release note: None 80511: pkg/cloud/azure: Support specifying Azure environments in storage URLs r=adityamaru a=nlowe-sx The Azure Storage cloud provider learned a new parameter, AZURE_ENVIRONMENT, which specifies which azure environment the storage account in question belongs to. This allows cockroach to backup and restore data to Azure Storage Accounts outside the main Azure Public Cloud. For backwards compatibility, this defaults to "AzurePublicCloud" if AZURE_ENVIRONMENT is not specified. Fixes cockroachdb#47163 ## Verification Evidence I spun up a single node cluster: ``` nlowe@nlowe-z4l:~/projects/github/cockroachdb/cockroach [feat/47163-azure-storage-support-multiple-environments L|✚ 2] [🗓 2022-04-22 08:25:49] $ bazel run //pkg/cmd/cockroach:cockroach -- start-single-node --insecure WARNING: Option 'host_javabase' is deprecated WARNING: Option 'javabase' is deprecated WARNING: Option 'host_java_toolchain' is deprecated WARNING: Option 'java_toolchain' is deprecated INFO: Invocation ID: 11504a98-f767-413a-8994-8f92793c2ecf INFO: Analyzed target //pkg/cmd/cockroach:cockroach (0 packages loaded, 0 targets configured). INFO: Found 1 target... Target //pkg/cmd/cockroach:cockroach up-to-date: _bazel/bin/pkg/cmd/cockroach/cockroach_/cockroach INFO: Elapsed time: 0.358s, Critical Path: 0.00s INFO: 1 process: 1 internal. INFO: Build completed successfully, 1 total action INFO: Build completed successfully, 1 total action * * WARNING: ALL SECURITY CONTROLS HAVE BEEN DISABLED! * * This mode is intended for non-production testing only. * * In this mode: * - Your cluster is open to any client that can access any of your IP addresses. * - Intruders with access to your machine or network can observe client-server traffic. * - Intruders can log in without password and read or write any data in the cluster. * - Intruders can consume all your server's resources and cause unavailability. * * * INFO: To start a secure server without mandating TLS for clients, * consider --accept-sql-without-tls instead. For other options, see: * * - https://go.crdb.dev/issue-v/53404/dev * - https://www.cockroachlabs.com/docs/dev/secure-a-cluster.html * * * WARNING: neither --listen-addr nor --advertise-addr was specified. * The server will advertise "nlowe-z4l" to other nodes, is this routable? * * Consider using: * - for local-only servers: --listen-addr=localhost * - for multi-node clusters: --advertise-addr=<host/IP addr> * * CockroachDB node starting at 2022-04-22 15:25:55.461315977 +0000 UTC (took 2.1s) build: CCL unknown @ (go1.17.6) webui: http://nlowe-z4l:8080/ sql: postgresql://root@nlowe-z4l:26257/defaultdb?sslmode=disable sql (JDBC): jdbc:postgresql://nlowe-z4l:26257/defaultdb?sslmode=disable&user=root RPC client flags: /home/nlowe/.cache/bazel/_bazel_nlowe/cf6ed4d0d14c8e474a5c30d572846d8a/execroot/cockroach/bazel-out/k8-fastbuild/bin/pkg/cmd/cockroach/cockroach_/cockroach <client cmd> --host=nlowe-z4l:26257 --insecure logs: /home/nlowe/.cache/bazel/_bazel_nlowe/cf6ed4d0d14c8e474a5c30d572846d8a/execroot/cockroach/bazel-out/k8-fastbuild/bin/pkg/cmd/cockroach/cockroach_/cockroach.runfiles/cockroach/cockroach-data/logs temp dir: /home/nlowe/.cache/bazel/_bazel_nlowe/cf6ed4d0d14c8e474a5c30d572846d8a/execroot/cockroach/bazel-out/k8-fastbuild/bin/pkg/cmd/cockroach/cockroach_/cockroach.runfiles/cockroach/cockroach-data/cockroach-temp4100501952 external I/O path: /home/nlowe/.cache/bazel/_bazel_nlowe/cf6ed4d0d14c8e474a5c30d572846d8a/execroot/cockroach/bazel-out/k8-fastbuild/bin/pkg/cmd/cockroach/cockroach_/cockroach.runfiles/cockroach/cockroach-data/extern store[0]: path=/home/nlowe/.cache/bazel/_bazel_nlowe/cf6ed4d0d14c8e474a5c30d572846d8a/execroot/cockroach/bazel-out/k8-fastbuild/bin/pkg/cmd/cockroach/cockroach_/cockroach.runfiles/cockroach/cockroach-data storage engine: pebble clusterID: bb3942d7-f241-4d26-aa4a-1bd0d6556e4d status: initialized new cluster nodeID: 1 ``` I was then able to view the contents of a backup hosted in an azure government storage account: ``` root@:26257/defaultdb> SELECT DISTINCT object_name FROM [SHOW BACKUP 'azure://container/path/to/backup?AZURE_ACCOUNT_NAME=account&AZURE_ACCOUNT_KEY=***&AZURE_ENVIRONMENT=AzureUSGovernmentCloud'] WHERE object_type = 'database'; object_name ------------------------------------------ example_database ... (17 rows) Time: 5.859632889s ``` Omitting the `AZURE_ENVIRONMENT` parameter, we can see cockroach defaults to the public cloud where my storage account does not exist: ``` root@:26257/defaultdb> SELECT DISTINCT object_name FROM [SHOW BACKUP 'azure://container/path/to/backup?AZURE_ACCOUNT_NAME=account&AZURE_ACCOUNT_KEY=***'] WHERE object_type = 'database'; ERROR: reading previous backup layers: unable to list files for specified blob: Get "https://account.blob.core.windows.net/container?comp=list&delimiter=path%2Fto%2Fbackup&restype=container&timeout=61": dial tcp: lookup account.blob.core.windows.net on 8.8.8.8:53: no such host ``` ## Tests Two new tests are added to verify that the storage account URL is correctly built from the provided Azure Environment name, and that the Environment defaults to the Public Cloud if unspecified for backwards compatibility. I verified the existing tests pass against a government storage account after specifying `AZURE_ENVIRONMENT` as `AzureUSGovernmentCloud` in the backup URL query parameters: ``` nlowe@nlowe-mbp:~/projects/github/cockroachdb/cockroachdb [feat/47163-azure-storage-support-multiple-environments| …3] [🗓 2022-04-22 17:38:26] $ export AZURE_ACCOUNT_NAME=account nlowe@nlowe-mbp:~/projects/github/cockroachdb/cockroachdb [feat/47163-azure-storage-support-multiple-environments| …3] [🗓 2022-04-22 17:38:42] $ export AZURE_ACCOUNT_KEY=*** nlowe@nlowe-mbp:~/projects/github/cockroachdb/cockroachdb [feat/47163-azure-storage-support-multiple-environments| …3] [🗓 2022-04-22 17:39:25] $ export AZURE_CONTAINER=container nlowe@nlowe-mbp:~/projects/github/cockroachdb/cockroachdb [feat/47163-azure-storage-support-multiple-environments| …3] [🗓 2022-04-22 17:39:48] $ export AZURE_ENVIRONMENT=AzureUSGovernmentCloud nlowe@nlowe-mbp:~/projects/github/cockroachdb/cockroachdb [feat/47163-azure-storage-support-multiple-environments| …3] [🗓 2022-04-22 17:40:15] $ bazel test --test_output=streamed --test_arg=-test.v --action_env=AZURE_ACCOUNT_NAME --action_env=AZURE_ACCOUNT_KEY --action_env=AZURE_CONTAINER --action_env=AZURE_ENVIRONMENT //pkg/cloud/azure:azure_test INFO: Invocation ID: aa88a942-f3c7-4df6-bade-8f5f0e18041f WARNING: Streamed test output requested. All tests will be run locally, without sharding, one at a time INFO: Build option --action_env has changed, discarding analysis cache. INFO: Analyzed target //pkg/cloud/azure:azure_test (468 packages loaded, 16382 targets configured). INFO: Found 1 test target... initialized metamorphic constant "span-reuse-rate" with value 28 === RUN TestAzure === RUN TestAzure/simple_round_trip === RUN TestAzure/exceeds-4mb-chunk === RUN TestAzure/exceeds-4mb-chunk/rand-readats === RUN TestAzure/exceeds-4mb-chunk/rand-readats/#00 cloud_test_helpers.go:226: read 3345 of file at 4778744 === RUN TestAzure/exceeds-4mb-chunk/rand-readats/#1 cloud_test_helpers.go:226: read 7228 of file at 226589 === RUN TestAzure/exceeds-4mb-chunk/rand-readats/#2 cloud_test_helpers.go:226: read 634 of file at 256284 === RUN TestAzure/exceeds-4mb-chunk/rand-readats/#3 cloud_test_helpers.go:226: read 7546 of file at 3546208 === RUN TestAzure/exceeds-4mb-chunk/rand-readats/#4 cloud_test_helpers.go:226: read 24123 of file at 4821795 === RUN TestAzure/exceeds-4mb-chunk/rand-readats/#5 cloud_test_helpers.go:226: read 16899 of file at 403428 === RUN TestAzure/exceeds-4mb-chunk/rand-readats/#6 cloud_test_helpers.go:226: read 29467 of file at 4886370 === RUN TestAzure/exceeds-4mb-chunk/rand-readats/#7 cloud_test_helpers.go:226: read 11700 of file at 1876920 === RUN TestAzure/exceeds-4mb-chunk/rand-readats/#8 cloud_test_helpers.go:226: read 2928 of file at 489781 === RUN TestAzure/exceeds-4mb-chunk/rand-readats/cockroachdb#9 cloud_test_helpers.go:226: read 19933 of file at 1483342 === RUN TestAzure/read-single-file-by-uri === RUN TestAzure/write-single-file-by-uri === RUN TestAzure/file-does-not-exist === RUN TestAzure/List === RUN TestAzure/List/root === RUN TestAzure/List/file-slash-numbers-slash === RUN TestAzure/List/root-slash === RUN TestAzure/List/file === RUN TestAzure/List/file-slash === RUN TestAzure/List/slash-f === RUN TestAzure/List/nothing === RUN TestAzure/List/delim-slash-file-slash === RUN TestAzure/List/delim-data --- PASS: TestAzure (34.81s) --- PASS: TestAzure/simple_round_trip (9.66s) --- PASS: TestAzure/exceeds-4mb-chunk (16.45s) --- PASS: TestAzure/exceeds-4mb-chunk/rand-readats (6.41s) --- PASS: TestAzure/exceeds-4mb-chunk/rand-readats/#00 (0.15s) --- PASS: TestAzure/exceeds-4mb-chunk/rand-readats/#1 (0.64s) --- PASS: TestAzure/exceeds-4mb-chunk/rand-readats/#2 (0.65s) --- PASS: TestAzure/exceeds-4mb-chunk/rand-readats/#3 (0.60s) --- PASS: TestAzure/exceeds-4mb-chunk/rand-readats/#4 (0.75s) --- PASS: TestAzure/exceeds-4mb-chunk/rand-readats/#5 (0.80s) --- PASS: TestAzure/exceeds-4mb-chunk/rand-readats/#6 (0.75s) --- PASS: TestAzure/exceeds-4mb-chunk/rand-readats/#7 (0.65s) --- PASS: TestAzure/exceeds-4mb-chunk/rand-readats/#8 (0.65s) --- PASS: TestAzure/exceeds-4mb-chunk/rand-readats/cockroachdb#9 (0.77s) --- PASS: TestAzure/read-single-file-by-uri (0.60s) --- PASS: TestAzure/write-single-file-by-uri (0.60s) --- PASS: TestAzure/file-does-not-exist (1.05s) --- PASS: TestAzure/List (2.40s) --- PASS: TestAzure/List/root (0.30s) --- PASS: TestAzure/List/file-slash-numbers-slash (0.30s) --- PASS: TestAzure/List/root-slash (0.30s) --- PASS: TestAzure/List/file (0.30s) --- PASS: TestAzure/List/file-slash (0.30s) --- PASS: TestAzure/List/slash-f (0.30s) --- PASS: TestAzure/List/nothing (0.15s) --- PASS: TestAzure/List/delim-slash-file-slash (0.15s) --- PASS: TestAzure/List/delim-data (0.30s) === RUN TestAntagonisticAzureRead --- PASS: TestAntagonisticAzureRead (103.90s) === RUN TestParseAzureURL === RUN TestParseAzureURL/Defaults_to_Public_Cloud_when_AZURE_ENVIRONEMNT_unset === RUN TestParseAzureURL/Can_Override_AZURE_ENVIRONMENT --- PASS: TestParseAzureURL (0.00s) --- PASS: TestParseAzureURL/Defaults_to_Public_Cloud_when_AZURE_ENVIRONEMNT_unset (0.00s) --- PASS: TestParseAzureURL/Can_Override_AZURE_ENVIRONMENT (0.00s) === RUN TestMakeAzureStorageURLFromEnvironment === RUN TestMakeAzureStorageURLFromEnvironment/AzurePublicCloud === RUN TestMakeAzureStorageURLFromEnvironment/AzureUSGovernmentCloud --- PASS: TestMakeAzureStorageURLFromEnvironment (0.00s) --- PASS: TestMakeAzureStorageURLFromEnvironment/AzurePublicCloud (0.00s) --- PASS: TestMakeAzureStorageURLFromEnvironment/AzureUSGovernmentCloud (0.00s) PASS Target //pkg/cloud/azure:azure_test up-to-date: _bazel/bin/pkg/cloud/azure/azure_test_/azure_test INFO: Elapsed time: 159.865s, Critical Path: 152.35s INFO: 66 processes: 2 internal, 64 darwin-sandbox. INFO: Build completed successfully, 66 total actions //pkg/cloud/azure:azure_test PASSED in 139.9s INFO: Build completed successfully, 66 total actions ``` 80705: kvclient: fix gRPC stream leak in rangefeed client r=tbg,srosenberg a=erikgrinaker When the DistSender rangefeed client received a `RangeFeedError` message and propagated a retryable error up the stack, it would fail to close the existing gRPC stream, causing stream/goroutine leaks. Release note (bug fix): Fixed a goroutine leak when internal rangefeed clients received certain kinds of retriable errors. 80762: joberror: add ConnectionReset/ConnectionRefused to retryable err allow list r=miretskiy a=adityamaru Bulk jobs will no longer treat `sysutil.IsErrConnectionReset` and `sysutil.IsErrConnectionRefused` as permanent errors. IMPORT, RESTORE and BACKUP will treat this error as transient and retry. Release note: None 80773: backupccl: break dependency to testcluster r=irfansharif a=irfansharif Noticed we were building testing library packages when building CRDB binaries. $ bazel query "somepath(//pkg/cmd/cockroach-short, //pkg/testutils/testcluster)" //pkg/cmd/cockroach-short:cockroach-short //pkg/cmd/cockroach-short:cockroach-short_lib //pkg/ccl:ccl //pkg/ccl/backupccl:backupccl //pkg/testutils/testcluster:testcluster Release note: None Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com> Co-authored-by: Nathan Lowe <nathan.lowe@spacex.com> Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com> Co-authored-by: Aditya Maru <adityamaru@gmail.com> Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>

itsbilal and others added 12 commits June 4, 2020 19:07

storage: Remove extra vars from runExportToSst

4f38f0e

A previous change (cockroachdb#49721) adding a benchmark for ExportToSst left in some extra unused variables in a helper method. This change cleans that up. Release note: None.

sql: implement ConstructPlan by new factory

3ea982a

This commit extracts the logic of `execFactory.ConstructPlan` into the function to be used by both factories. It also extracts out another small helper. Release note: None

sql: populate spatial_ref_sys table with projection entries

2949573

Also fixed a bug where `\0` didn't actually output a NULL terminator (no idea why it worked). Release note (sql change): Populate the spatial_ref_sys table with support SRID entries for geospatial data types.

sql: use nicer apd.Decimal.SetInt64 in the code base

3763073

This commit is automatic replacement of `apd.Decimal.SetFinite((.*), 0)` into a nicer `apd.Decimal.SetInt64($1)` which are equivalent. Release note: None

geo: move TestClipRectByEWKT test to geoindex

8dfdf3a

Trying to make the geo package not depend on GEOS, and this is one of the required steps. Release note: None

colexec: don't randomize batch size in benches

13845e4

This was making it hard to run the same benchmarks in a consistent way. Release note: None

colexec: remove unnecessary execgen imports

269f635

The .eg.go files had unnecessary imports of execgen, which is not required in the runtime generated code, and in fact is suspicious. Release note: None

scrips/release-notes: fix the error encountered on cockroachdb#47761

4541522

There's at lease one PR with a missing branch tips in refs/pull. This commit makes the script tolerant of that failure. Release note: None

ajwerner force-pushed the ajwerner/wrapped-descriptors branch from 841534e to aab9db4 Compare June 8, 2020 14:35

jobs: rename Resume to Unpause

a1028aa

Resume verb is already used to for the action of starting a job after adoption on a node. Here instead Resume is the opposite action of pausing a job. Release note: none.

ajwerner force-pushed the ajwerner/wrapped-descriptors branch from aab9db4 to 17286be Compare June 8, 2020 14:37

ajwerner force-pushed the ajwerner/wrapped-descriptors branch from 17286be to 1f02f8f Compare June 8, 2020 15:00

ajwerner force-pushed the ajwerner/wrapped-descriptors branch from 1f02f8f to de1ec78 Compare June 8, 2020 15:08

yuzefovich added 2 commits June 8, 2020 08:18

ajwerner force-pushed the ajwerner/wrapped-descriptors branch from de1ec78 to db3a0bf Compare June 8, 2020 15:31

rytaft and others added 6 commits June 8, 2020 10:36

lint: fix a linter message

b0a478a

A message had an unescaped format and ended up rendered like: use YourFuncf("descriptive prefix %!s(MISSING)", ...) Release note: None

ajwerner force-pushed the ajwerner/wrapped-descriptors branch from db3a0bf to 55273b3 Compare June 8, 2020 18:23

craig[bot] and others added 5 commits June 8, 2020 21:59

Merge cockroachdb#49949

cb34350

49949: geo/geogfn: implement ST_Project r=otan a=hueypark Fixes cockroachdb#48402 Release note (sql change): This PR implement adds the ST_Project({geography,float8,float8}) Co-authored-by: Jaewan Park <jaewan.huey.park@gmail.com>

ajwerner force-pushed the ajwerner/wrapped-descriptors branch from 55273b3 to df92ae2 Compare June 9, 2020 01:42

jordanlewis and others added 4 commits June 8, 2020 22:36

geo: handle maximum decimal digits for ST_AsText

e7b0c76

Release note (sql change): Introduce maxDecimalDigits arguments for ST_AsText and ST_AsEWKT, which allow rounding of the decimal digits output in the WKT representation.

ajwerner force-pushed the ajwerner/wrapped-descriptors branch from df92ae2 to 237d2d0 Compare June 9, 2020 03:48

koorosh and others added 16 commits June 9, 2020 11:27

ui: CSS modules for Statements page filters

8677bc0

This change refactors following components to use CSS modules instead of styles defined as global: - Dropdown - Search - PageConfig Release note: None

sqlbase,catalog: Move the catalog.Descriptor interface to sqlbase

bf18d34

For now, move the Descriptor interface down into sqlbase as part of the effort to elimate sqlbase.DescriptorProto. Release note: None

sql,sqlbase: remove SetID() from the DescriptorProto interface

0f3039f

In preparation for merging that interface with DescriptorInterface. Release note: None

sqlbase: introduce DatabaseDescriptorInterface and ImmutableDatabaseD…

96411bf

…escriptor Next we'll stop having the raw descriptor implement that interface. Release note: None

sqlbase: lift methods from TypeDescriptor to ImmutableTypeDescriptor

cbde216

Release note: None

sqlbase: add wrapper types for SchemaDescriptor

7c86629

In anticipation of lifting methods to the wrapper types. Release note: None

sqlbase: lift interface methods from SchemaDescriptor

1589d99

Release note: None

ajwerner force-pushed the ajwerner/wrapped-descriptors branch from 237d2d0 to d5229de Compare June 9, 2020 13:51

ajwerner merged commit 86c4fbd into master Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ajwerner/wrapped descriptors #6

Ajwerner/wrapped descriptors #6

ajwerner commented Jun 8, 2020

Ajwerner/wrapped descriptors #6

Ajwerner/wrapped descriptors #6

Conversation

ajwerner commented Jun 8, 2020