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

[WIP] sql: pass descs.Collection to InternalExecutor under planner context #80262

Closed

Conversation

ZhouXing19
Copy link
Collaborator

@ZhouXing19 ZhouXing19 commented Apr 20, 2022

WIP

Currently, when an internal executor is used inside a transaction
under a planner context, it always creates a new descriptor collection
rather than inheriting from the outer transaction.

We'd like to pass desc.Collection to an internal executor ONLY when
the internal executor is used under the planner context.
To do this, we

  1. introduced a type extraTxnStateUnderPlanner, which is only accessible
    under the sql package;
  2. deprecate ExecutorConfig.InternalExecutorFactory, and break the internal
    executor initialization into 2 steps:
    a. under newSQLServer(), we initialize an InternalExecutorProto, which
    cannot be used to execute queries;
    b. initialize the true internal executor with desc.Collection via
    extraTxnStateUnderPlanner, if it's under the planner context.

Fixes #69495

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ZhouXing19 ZhouXing19 requested a review from rafiss April 21, 2022 18:39
@ZhouXing19 ZhouXing19 force-pushed the internal-executor-collection branch 4 times, most recently from efecba9 to 97ed700 Compare April 27, 2022 20:11
@ZhouXing19 ZhouXing19 changed the title [WIP] sql: pass descs.Collection to InternalExecutor for initialization sql: pass descs.Collection to InternalExecutor for initialization Apr 27, 2022
@ZhouXing19 ZhouXing19 force-pushed the internal-executor-collection branch 2 times, most recently from 7ce154a to e0d899f Compare April 27, 2022 20:18
@ZhouXing19 ZhouXing19 marked this pull request as ready for review April 27, 2022 20:18
@ZhouXing19 ZhouXing19 requested a review from a team as a code owner April 27, 2022 20:18
@ZhouXing19 ZhouXing19 requested a review from a team April 27, 2022 20:18
@ZhouXing19 ZhouXing19 requested a review from a team as a code owner April 27, 2022 20:18
@ZhouXing19 ZhouXing19 requested review from a team April 27, 2022 20:18
@ZhouXing19 ZhouXing19 requested a review from a team as a code owner April 27, 2022 20:18
@ZhouXing19 ZhouXing19 requested review from shermanCRL and a team and removed request for a team April 27, 2022 20:18
@shermanCRL shermanCRL removed their request for review April 27, 2022 20:27
@ajwerner
Copy link
Contributor

This is neat. This change is very good for negating the efficiency problems we run into when we construct new internal executors for querying virtual tables, but it doesn't really mitigate issues like we saw in #76764. In order to have those work correctly we'd need to initialize and share also these parts of state:

// jobs accumulates jobs staged for execution inside the transaction.
// Staging happens when executing statements that are implemented with a
// job. The jobs are staged via the function QueueJob in
// pkg/sql/planner.go. The staged jobs are executed once the transaction
// that staged them commits.
jobs jobsCollection
// schemaChangeJobRecords is a map of descriptor IDs to job Records.
// Used in createOrUpdateSchemaChangeJob so we can check if a job has been
// queued up for the given ID. The cache remains valid only for the current
// transaction and it is cleared after the transaction is committed.
schemaChangeJobRecords map[descpb.ID]*jobs.Record

In reality, I think sharing the descs.Collection for the purpose of writing is a bad idea. It's really good for when we're using the (*planner).QueryRowEx/QueryIteratorEx but I'm suspicious of it in other use cases. I'm worried that this new interface surface area may give folks a false sense of security that if they do inject a descs.Collection then they do can do anything with an internal executor.

My preference would be that we find a way to hide the surface area more, and that we disable all schema changes using an internal executor with an explicit transactions, and then we just do this coupling for the internal executors accessed via the planner interface used in the EvalContext.

Does that seem reasonable?

@ZhouXing19 ZhouXing19 marked this pull request as draft May 4, 2022 19:14
@ZhouXing19 ZhouXing19 changed the title sql: pass descs.Collection to InternalExecutor for initialization [WIP] sql: pass descs.Collection to InternalExecutor under planner context May 4, 2022
@ZhouXing19 ZhouXing19 force-pushed the internal-executor-collection branch 3 times, most recently from 51926ad to 7630e51 Compare May 5, 2022 17:33
@otan otan removed request for a team May 12, 2022 23:33
@otan otan removed request for a team May 12, 2022 23:33
@ZhouXing19 ZhouXing19 force-pushed the internal-executor-collection branch 7 times, most recently from a3db917 to 0137d18 Compare May 19, 2022 18:31
@ZhouXing19 ZhouXing19 force-pushed the internal-executor-collection branch from 66dbda3 to c046563 Compare May 23, 2022 15:34
@ZhouXing19
Copy link
Collaborator Author

Note to myself:
Now we restrict the usage makeSessionBoundInternalExecutorFromProtoUnderPlanner to under sql package, which is good -- if an ie is used from any other packages to run queries, ie.extraTxnState cannot be set.

The problem unsolved is that if under a planner context, the ie can still be initialized with MakeSessionBoundedInternalExecutorFromProto, but they should ALWAYS be inited with makeSessionBoundInternalExecutorFromProtoUnderPlanner.

I.e. we want to make the usage of makeSessionBoundInternalExecutorFromProtoUnderPlanner and MakeSessionBoundedInternalExecutorFromProto more "separate" -- if under a planner context, only makeSessionBoundInternalExecutorFromProtoUnderPlanner is allowed; otherwise, only MakeSessionBoundedInternalExecutorFromProto is allowed.

Currently, when an internal executor is used inside a transaction
under a planner context, it always creates a new descriptor collection
rather than inheriting from the outer transaction.

We'd like to pass `desc.Collection` to an internal executor ONLY when
the internal executor is used under the planner context.

To do this, we

1. introduced a type `extraTxnStateUnderPlanner`, which is only accessible
under the sql package;

2. deprecate `ExecutorConfig.InternalExecutorFactory`, and break the internal
executor initialization into 2 steps:

	a. under newSQLServer(), we initialize an `InternalExecutorProto`, which
	cannot be used to execute queries;

	b. initialize the true internal executor with `desc.Collection` via
	extraTxnStateUnderPlanner, if it's under the planner context.

Fixes cockroachdb#69495

Release note: None
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.

sql: make InternalExecutor have the same descs.Collection as the parent
3 participants