sql: improve interface for injecting descriptors into internal executor#58906
sql: improve interface for injecting descriptors into internal executor#58906craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
b3557a6 to
ff56d1d
Compare
|
This could maybe use a few tests, but it's ready for a look. This interface could certainly be improved further, but I figure we'll need to expand the synthetic descriptor concept in new ways for transactional schema changes, and further movement in that direction now would be premature. |
ff56d1d to
20fb20b
Compare
ajwerner
left a comment
There was a problem hiding this comment.
This is definitely better.
Reviewed 3 of 4 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)
pkg/sql/catalog/descs/collection.go, line 361 at r1 (raw file):
getDatabaseByName := func() (found bool, _ catalog.Descriptor, err error) { if found, sd := tc.getSyntheticDescriptorByName(
could we pull the logic to retreive the synthetic descriptor underneath the logic to get uncommitted descriptors?
d2f8858 to
e31abe0
Compare
0708b77 to
69cb576
Compare
thoszhang
left a comment
There was a problem hiding this comment.
Added a basic test.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/sql/catalog/descs/collection.go, line 361 at r1 (raw file):
Previously, ajwerner wrote…
could we pull the logic to retreive the synthetic descriptor underneath the logic to get uncommitted descriptors?
I did this for the resolution-by-name path. I think I'll do it for the by-ID path as part of #57343 once this merges.
fbe07a9 to
c692ec6
Compare
thoszhang
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/sql/catalog/descs/collection.go, line 361 at r1 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
I did this for the resolution-by-name path. I think I'll do it for the by-ID path as part of #57343 once this merges.
Done.
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. Release note: None
c692ec6 to
77cfe96
Compare
|
I rebased on top of some recent changes. This is RFAL. |
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2, 1 of 5 files at r3.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)
|
TFTR bors r+ |
|
Build succeeded: |
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.Collectionand adding the injected descriptors as uncommitteddescriptors, and then setting this collection as the
tcModifieron theinternal executor. Then all subsequent queries using the internal
executor, which each get their own child
connExecutoranddescs.Collection, will have their collection's uncommitted descriptorsreplaced 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:
descs.Collectionto hold syntheticdescriptors to copy, we now just use a slice of
catalog.Descriptors.descs.Collectionand 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.Collectionare seton 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