*: migrate the creation of internal executor to the new interfaces #91004

ZhouXing19 · 2022-10-31T17:56:42Z

TL;DR:

To run multiple SQL statements and/or DDL statements in a transactional manner, use sql.planner.WithInternalExecutor(), sqlutil.InternalExecutorFactory.TxnWithExecutor(), or descs.CollectionFactory.TxnWithExecutor(). (example)
To run other single statements under a planner context, use query functions under planner (e.g. sql.planner.QueryRowEx() or sql.planner.WithInternalExecutor())
Otherwise, use sqlutil.InternalExecutorFactory.MakeInternalExecutorWithoutTxn().

We'd like to migrate the existing usages of internal executor to the interfaces above. Once these are all using the new interfaces, we will remove the txn parameter in the query functions (e.g. ie.ExecEx()) and use ie.extraTxnState.txn.

Motivation

The previous design of the internal executor is either incorrect or inefficient when running with an outer txn, as it always creates a new set of txn-related metadata (such as descriptor collection and schema change job records) for its child conn executor. This is not intuitive, since it violates a rather deep principle that these metadata and a txn have a 1:1 relationship. The lack of such coupling has caused issues such as filetable corruption, unnecessary rescanning of the descriptors, etc.

New interfaces

The new internal executor interfaces (implemented in #82477, #86427, and parts of #87067) are meant to solve this problem. They ensure that the internal executor, if used with an outer txn, is bound with the correct metadata, which will be passed to the child conn executor. When the internal executor is closed, the sql transaction will be committed.

Epic: CRDB-19135

Jira issue: CRDB-21069

Epic CRDB-19135

91186: sql: let `extendedEvalContext.QueueJob()` use txn from planner r=ZhouXing19 a=ZhouXing19 This commit is part of the effort to reduce usages of `eval.Context.txn`. We now let `*kv.Txn` be passed in as a parameter to `extendedEvalContext.QueueJob()`. Informs: #91004 Release note: None Co-authored-by: Jane Xing <zhouxing@uchicago.edu>

90964: streamingccl: avoid passing `evalCtx`, `txn` as parameters to ingestion & replication funcs r=ZhouXing19 a=ZhouXing19 This PR is part of the effort to eliminate usages of `eval.Context.Txn`. It moves 1. the definition of `StreamIngestionManager` and `ReplicationStreamManager` under eval; 2. the implementation of `StreamIngestionManager` and `ReplicationStreamManager` via `sql.planner`. The core changes are ``` // GetReplicationStreamManager returns a ReplicationStreamManager. func (p *planner) GetReplicationStreamManager( ctx context.Context, ) (eval.ReplicationStreamManager, error) { return streaming.GetReplicationStreamManager(ctx, p.EvalContext(), p.Txn()) } // GetStreamIngestManager returns a StreamIngestManager. func (p *planner) GetStreamIngestManager(ctx context.Context) (eval.StreamIngestManager, error) { return streaming.GetStreamIngestManager(ctx, p.EvalContext(), p.Txn()) } ``` so that the functions under these 2 interfaces run upon `eval.Context` and `kv.Txn` from the `sql.planner`. Follow-up: - [ ] Pass internal executor from planner too, rather than using `register.ex`. informs #90923 91249: acceptance: deflake `TestDockerCLI/test_txn_prompt` r=rafiss a=renatolabs That test would sometimes fail because of the semantics of `expect` and `send` when the expected string was previously written using `send`. When `expect` is called, the buffer looked at includes content previously sent using `send`. This means that if one runs `send "foo"; expect foo`, the `expect` call will match instataneously even if the program's output after the send does not contain `foo`. In the case of the test fixed here, we are supposed to expect for the new prompt to be printed after setting it with `\set prompt1`. In order to properly check whether the prompt changed, this PR changes the prompt `expect` call to use a regular expression that matches the new prompt only if it sits in the beginning of a line. Prior to this commit, since the `expect` call would return immediately, there was a chance the `send "SET DATABASE"` command could run before the cockroach CLI had the chance to print the new prompt, leading to the following error: ``` abcdefaultdbdef \set prompt1 woo SET database woo -> .221103 18:13:35.539667600 EXPECT TEST: TIMEOUT WAITING FOR " -> " non-zero exit code: 1 ``` Epic: None Release note: None 91401: spanconfigsqltranslator: add sqlutil.InternalExecutor to SQLTranslator r=arulajmani a=ZhouXing19 This commit is part of the effort of having an internal executor better bound to its outer txn if there's one. The next step after this commit is to replace the executor used in `s.ptsProvider.GetState()` in `SQLTranslator.Translate()` to the one hanging off `SQLTranslator`. Informs: #91004 Release note: None 91423: roachpb: fix bug when logging lease in NLE r=ajwerner a=ajwerner We were logging `lease holder unknown` when the deprecated field was not populated. Epic: None Release note: None 91436: multitenant: add admin function `RangeIterator failed to seek` test cases r=rafiss a=ecwall refs #91434 This change adds test cases for admin functions (see #91434) that fail because of a `RangeIterator failed to seek` error once the multitenant check is bypassed. This needs to be addressed before those admin functions can be supported for secondary tenants. Release note: None 91508: logictest: fix flake in fk due to sequence non-determinism r=ajwerner a=ajwerner See [here](https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_BazelEssentialCi/7392167?showRootCauses=false&expandBuildChangesSection=true&expandBuildProblemsSection=true&expandBuildTestsSection=true). This patch stops showing the sequence column while still relying on its ordering properties. Epic: None Release note: None 91515: kvserver: return DeprecatedLeaseHolder field in NLHEs r=arulajmani a=arulajmani v22.1 binaries assume that the leaseholder is unknown when logging NLHE errors if the (Deprecated)LeaseHolder field is unset -- regardless of if the Lease is set or not. We broke this logging in 0402f47 (for mixed version clusters) when we stopped shipping back leaseholder information (in favour of only shipping lease information) on NLHEs. This patch fixes this by populating the (Deprecated)LeaseHolder field when constructing NLHEs. Release note: None Co-authored-by: Jane Xing <zhouxing@uchicago.edu> Co-authored-by: Renato Costa <renato@cockroachlabs.com> Co-authored-by: Andrew Werner <awerner32@gmail.com> Co-authored-by: Evan Wall <wall@cockroachlabs.com> Co-authored-by: Arul Ajmani <arulajmani@gmail.com>

When creating a job, we should use an internal executor bound to the outer txn and related metadata. We also found that in this case, if a user changes the timezone and then triggers a schema change job, the job's creation time (i.e. value createdcolumn in the system.jobs) will automatically switch to the timestamp in the updated timezone, rather than UTC. This is because we have the created using now() in the SQL statement. We now change it to be inserted always with UTC time. Informs: cockroachdb#91004 Informs: cockroachdb#91718 Release note: None

91679: jobs: replace the ie in Registry.createJobsInBatchWithTxn r=ajwerner a=ZhouXing19 When creating a job, we should use an internal executor bound to the outer txn and related metadata. We also found that in this case, if a user changes the timezone and then triggers a schema change job, the job's creation time (i.e. value `created`column in the `system.jobs`) will automatically switch to the timestamp in the updated timezone, rather than UTC. This is because we have the `created` using `now()` in the SQL statement. We now change it to be inserted always with UTC time. Informs: #91004 Informs: #91718 Release note: None 91734: kvserver: move checkForcedErr to kvserverbase r=pavelkalinnikov a=tbg This is a prereq to making log application stand-alone, since applying entries means calling this method to determine whether to apply an empty entry instead. This necessitated moving the constructor for NotLeaseholderError where it belongs (roachpb) and moving proposal evaluation reasons to somewhere else (I chose kvserverpb). While I was there I also renamed their type to better reflect that these are command rejections. Touches #75729. Epic: CRDB-220 Release note: None 91772: roachtest: fix typo when saving artifacts in schemachange_random_load r=postamar,ajwerner a=renatolabs Epic: None Release note: None Co-authored-by: Jane Xing <zhouxing@uchicago.edu> Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com> Co-authored-by: Renato Costa <renato@cockroachlabs.com>

This commit is to use the new internal executor interface introduced by cockroachdb#93218 for `schemachanger.txn()`. informs cockroachdb#91004 Release Note: None

95963: kvserver: use narrow checkpoints in consistency checker r=tbg,erikgrinaker a=pavelkalinnikov This commit modifies the consistency checker to save partial checkpoints instead of full ones. The partial checkpoints contain the data for the inconsistent range and its immediate left and right neighbour in the Store. Only the inconsistent range's Replica is locked at the time of snapshotting the store before creating the checkpoint, other ranges can be out of sync. They are checkpointed too, to give more context when e.g. debugging non-trivial split/merge scenarios. Release note (ops change): In the rare event of Range inconsistency, the consistency checker saves a storage checkpoint on each storage the Range belongs to. Previously, this was a full checkpoint, so its cost could quickly escalate on the nodes that went on running. This change makes the checkpoints partial, i.e. they now only contain the relevant range and its neighbours. This eliminates the time pressure on the cluster operator to remove the checkpoints. Resolves #90543 Epic: none 96243: schemachanger: Implement `ADD CONSTRAINT NOT VALID` r=Xiang-Gu a=Xiang-Gu This PR implements the following three statements in the declarative schema changer: - `ALTER TABLE ... ADD CHECK ... NOT VALID` - `ALTER TABLE ... ADD UNIQUE WITHOUT INDEX ... NOT VALID` - `ALTER TABLE ... ADD FOREIGN KEY ... NOT VALID` The idea is to introduce a new element for each statement. That element is like simple dependent and will transition between ABSENT and PUBLIC directly. Epic: None 96598: sql: use `descs.Txn` for `schemachanger.txn` r=ajwerner a=ZhouXing19 This commit includes just mechanical changes to use the new internal executor interface introduced by #93218 for `schemachanger.txn()`. informs #91004 Release Note: None Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com> Co-authored-by: Xiang Gu <xiang@cockroachlabs.com> Co-authored-by: Jane Xing <zhouxing@uchicago.edu>

rimadeodhar · 2023-04-11T18:48:35Z

Synced with @ajwerner who confirmed that this issue has been completed by PR #93218. I'm closing out this issue.

ZhouXing19 added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Oct 31, 2022

exalate-issue-sync bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Oct 31, 2022

exalate-issue-sync bot assigned ZhouXing19 Oct 31, 2022

ZhouXing19 mentioned this issue Nov 3, 2022

sql: let extendedEvalContext.QueueJob() use txn from planner #91186

Merged

This was referenced Nov 7, 2022

release-22.2: sql: let extendedEvalContext.QueueJob() use txn from planner #91402

Closed

spanconfigsqltranslator: add sqlutil.InternalExecutor to SQLTranslator #91401

Merged

storage: add ie to storage.GetState and GetMetadata #91409

Closed

This was referenced Dec 2, 2022

upgrades: migrate the usages of TenantDeps.InternalExecutor to new interface #92938

Open

sql: add role_id and member_id columns to system.role_members table #91517

Merged

ZhouXing19 mentioned this issue Feb 6, 2023

sql: use descs.Txn for schemachanger.txn #96598

Merged

rimadeodhar closed this as completed Apr 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: migrate the creation of internal executor to the new interfaces #91004

*: migrate the creation of internal executor to the new interfaces #91004

ZhouXing19 commented Oct 31, 2022 •

edited by exalate-issue-sync bot

Loading

rimadeodhar commented Apr 11, 2023

*: migrate the creation of internal executor to the new interfaces #91004

*: migrate the creation of internal executor to the new interfaces #91004

Comments

ZhouXing19 commented Oct 31, 2022 • edited by exalate-issue-sync bot Loading

Motivation

New interfaces

rimadeodhar commented Apr 11, 2023

ZhouXing19 commented Oct 31, 2022 •

edited by exalate-issue-sync bot

Loading