sql: allow the InternalExecutor to work with a modified schema #34304
Labels
A-schema-changes
A-schema-descriptors
Relating to SQL table/db descriptor handling.
C-enhancement
Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
T-sql-foundations
SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
Comments
|
A conversation with @jordanlewis reminded me that if a transaction has a schema change, and a subsequent statement invokes an InternalExecutor, the InternalExecutor doesn't share the TableCollection state of the parent transaction, thus the InternalExecutor can deadlock with the parent transaction over a read to the table descriptor. |
|
@lucy-zhang FYI |
knz
added
C-enhancement
|
I've been able to make some progress on this. I was concerned that we might not be able to fix it, but the problem does look tractable on one of my branches. |
vivekmenezes
added a commit
to vivekmenezes/cockroach
that referenced
this issue
Jan 30, 2019
The internal executor uses its own TableCollection. This is fine normally but is a problem when it is called in the context of a transaction that has already modified schema. The parent transaction will leave an outstanding intent on the schema, and the InternalExecutor will block attempting to acquire a lease on the schema. The fix is to modify the TableCollection used by the InternalExecutor to house the modified schema so that a schema lookup made by the InternalExecutor will not attempt to acquire a lease. This change is only creating the infrastructure by which this is done but not fixing the underlying deadlock problem. related to cockroachdb#34304 Release note: None
vivekmenezes
added a commit
to vivekmenezes/cockroach
that referenced
this issue
Feb 6, 2019
The internal executor uses its own TableCollection. This is fine normally but is a problem when it is called in the context of a transaction that has already modified schema. The parent transaction will leave an outstanding intent on the schema, and the InternalExecutor will block attempting to acquire a lease on the schema. The fix is to modify the TableCollection used by the InternalExecutor to house the modified schema so that a schema lookup made by the InternalExecutor will not attempt to acquire a lease. This change is only creating the infrastructure by which this is done but not fixing the underlying deadlock problem. related to cockroachdb#34304 Release note: None
vivekmenezes
changed the title
dt
removed this from SQL Descriptor Mgmt (might be SQL-Exec issues?)
in Disaster Recovery Backlog
|
#58906 improves the interface slightly. An even better step would be to have the synthetic descriptors be scoped at a per-query/statement level (i.e., for each call to |
craig bot
pushed a commit
that referenced
this issue
Feb 1, 2021
58436: server: add /api/v2/ tree with auth/pagination, port listSessions endpoint r=itsbilal a=itsbilal This change adds the skeleton for a new API tree that lives in `/api/v2/` in the http listener, and currently reimplements the `/sessions/` endpoint that is also implemented in `/_status/`. The new v2 API tree avoids the need to use GRPC Gateway, as well as cookie-based authentication which is less intuitive and idiomatic for REST APIs. Instead, for authentication, it uses a new session header that needs to be set on every request. As many RPC fan-out APIs use statusServer.iterateNodes, this change implements a pagination-aware method, paginatedIterateNodes, that works on a sorted set of node IDs and arranges results in such a way to be able to return the next `limit` results of an arbitary slice after the `next` cursor passed in. An example of how this works in practice is the new `/api/v2/sessions/` endpoint. A dependency on gorilla/mux is added to be able to pattern-match arguments in the URL. This was already an indirect dependency; now it's a direct dependency of cockroach. TODO that are likely to fall over into future PRs: - API Documentation, need to explore using swagger here. - Porting over remaining /_admin/ and /_status/ APIs, incl. SQL based ones Part of #55947. Release note (api change): Adds a new API tree, in /api/v2/*, currently undocumented, that avoids the use of and cookie-based authentication in favour of sessions in headers, and support for pagination. 58906: sql: improve interface for injecting descriptors into internal executor r=lucy-zhang a=lucy-zhang As part of validating new schema elements (unique indexes, constraints) in the schema changer, we inject in-memory descriptors to be used by the internal executor that are never written to disk. This is currently done by creating an entirely new dummy `descs.Collection` and adding the injected descriptors as uncommitted descriptors, and then setting this collection as the `tcModifier` on the internal executor. Then all subsequent queries using the internal executor, which each get their own child `connExecutor` and `descs.Collection`, will have their collection's uncommitted descriptors replaced by the ones in the dummy collection. This commit introduces the concept of a "synthetic descriptor" to refer to these injected descriptors, and slightly improves the interface in two ways: 1. Instead of creating a new `descs.Collection` to hold synthetic descriptors to copy, we now just use a slice of `catalog.Descriptor`s. 2. Synthetic descriptors are now made explicit in the `descs.Collection` and precede uncommitted descriptors when resolving immutable descriptors. Resolving these descriptors mutably is now illegal and will return an assertion error. This commit doesn't change the fact that the synthetic descriptors to be injected into each query/statement's child `descs.Collection` are set on the internal executor itself. This is still not threadsafe. It would make more sense for these descriptors to be scoped at the level of individual queries. Related to #34304. Release note: None 59258: changefeedccl: Freeze table name to the (optionally fully qualified) statement time name r=[miretskiy] a=HonoreDB Previously, Kafka topics and top-level keys were always derived from the table name in changefeeds. If the table name changed, the feed eventually failed, and if the table name was non-unique across databases, collisions were unavoidable. This PR adds a WITH full_table_name option to changefeeds, and honors it by serializing movr.public.drivers as the statement time name and relying on that. There are probably more things that need to change downstream. Release note (sql change): Added "WITH full_table_name" option to create a changefeed on "movr.public.drivers" instead of "drivers". 59281: sql: prevent DROP SCHEMA CASCADE from droping types with references r=ajwerner a=ajwerner Before this patch, a DROP SCHEMA CASCADE could cause database corruption by dropping types which were referenced by other tables. This would lead to bad errors like: ``` ERROR: object already exists: desc 687: type ID 685 in descriptor not found: descriptor not found SQLSTATE: 42710 ``` And doctor errors like: ``` Table 687: ParentID 50, ParentSchemaID 29, Name 't': type ID 685 in descriptor not found: descriptor not found ``` Fixes #59021. Release note (bug fix): Fixed a bug where `DROP SCHEMA ... CASCADE` could result in types which are referenced being dropped. 59591: sql: hook up multi-region DDL to new zone config attributes r=aayushshah15 a=aayushshah15 After the introduction of #57184, we can constrain voting replicas separately from non-voting replicas using the new attributes `num_voters` and `voter_constraints`. This commit connects our multi-region DDL statements to leverage these new attributes. Broadly speaking, - The existing `constraints` and `num_replicas` fields are set at the database level, to be inherited by table/partition level zone configs. - The new attributes: `num_voters` and `voter_constraints` (along with accompanying `lease_preferences` for these voters) are set at the table/partition level. This brings the implementation of these DDL statements inline with the functional specfication. Fixes #57663 Release note: None 59659: sql: fix bug preventing adding FKs referencing hidden columns r=lucy-zhang a=lucy-zhang The validation query for adding foreign keys had a pointless `SELECT *` on the referenced table that caused hidden columns to be omitted, so attempting to add foreign key constraints referencing hidden columns would fail. This PR fixes the query. Fixes #59582. Release note (bug fix): Fixed a bug preventing foreign key constraints referencing hidden columns (e.g., `rowid`) from being added. Co-authored-by: Bilal Akhtar <bilal@cockroachlabs.com> Co-authored-by: Lucy Zhang <lucy@cockroachlabs.com> Co-authored-by: Aaron Zinger <zinger@cockroachlabs.com> Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com> Co-authored-by: Aayush Shah <aayush.shah15@gmail.com>
|
Synthetic descriptors do the job nicely. Closing. |
exalate-issue-sync
bot
added
T-sql-foundations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
There are at least 3 applications for schema change validation that require the validation to be done before the schema change goes public, but the validation is done using SQL that can only function on the public schema. What makes sense is to hand a schema from the future to the internal executor so that it can do the right thing.
Jira issue: CRDB-4655
The text was updated successfully, but these errors were encountered: