Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: improve interface for injecting descriptors into internal executor #58906

Merged
merged 1 commit into from Feb 1, 2021

Conversation

thoszhang
Copy link
Contributor

@thoszhang thoszhang commented Jan 13, 2021

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.Descriptors.
  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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@thoszhang thoszhang force-pushed the improve-tcmodifier branch 3 times, most recently from b3557a6 to ff56d1d Compare January 13, 2021 18:18
@thoszhang thoszhang changed the title [wip] sql: improve interface for injecting synthetic descriptors into internal executor sql: improve interface for injecting descriptors into internal executor Jan 13, 2021
@thoszhang thoszhang marked this pull request as ready for review January 13, 2021 18:19
@thoszhang
Copy link
Contributor Author

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.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely better.

Reviewed 3 of 4 files at r1.
Reviewable status: :shipit: 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?

Copy link
Contributor Author

@thoszhang thoszhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a basic test.

Reviewable status: :shipit: 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.

@thoszhang thoszhang force-pushed the improve-tcmodifier branch 3 times, most recently from fbe07a9 to c692ec6 Compare January 15, 2021 04:51
Copy link
Contributor Author

@thoszhang thoszhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: 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
@thoszhang
Copy link
Contributor Author

I rebased on top of some recent changes. This is RFAL.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 2 files at r2, 1 of 5 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

@thoszhang
Copy link
Contributor Author

TFTR

bors r+

@craig craig bot merged commit f37712a into cockroachdb:master Feb 1, 2021
@craig
Copy link
Contributor

craig bot commented Feb 1, 2021

Build succeeded:

@thoszhang thoszhang deleted the improve-tcmodifier branch February 4, 2021 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants