*: add restriction to running DDL with internal executors #86334

ZhouXing19 · 2022-08-17T20:51:01Z

The current internal executor has its lifecycle, which makes it erroneous
when being used to execute DDL statements if under an outer txn.

In this commit, we

Migrated the existing DDLs with internal executor with not-nil txn to either
descs.CollectionFactory.TxnWithExecutor() or planner.WithInternalExecutor().
Only internal executors inited via these 2 interfaces are bounded with txn-related
metadata, and hence are allowed to run DDLs in a transactional manner.
Added a restriction for running DDLs with internal executor only if it's bound with
txn-related metadata.

fixes #87281

Release justification: bug fix for the internal executor
Release note: none

cockroach-teamcity · 2022-08-17T20:51:09Z

This change is

ajwerner · 2022-08-17T20:52:19Z

What should we do if the internal executor was constructed with a transaction and then a non-nil transaction is passed in?

ZhouXing19 · 2022-08-17T21:03:22Z

What should we do if the internal executor was constructed with a transaction and then a non-nil transaction is passed in?

In the old design (prior to #82477), we don't construct an internal executor with a txn. Each time a not-nil txn passed in with a statement, we construct a conn executor with that txn. The internal executor is not bound to any txn at all.

In the new interfaces, we actually didn't truly used the txn bounded to the internal executor (ie.extraTxnState.txn), except for InternalExecutor.commitTxn(), which is only used under the collectionFactory. So essentially, for most of the cases, the txn that the conn executor is constructed with is still the one from the Ex functions.

Ideally, we should disallow passing a txn through the Ex functions, and always use ie.extraTxnState.txn. But the prerequisite is that all internal executor usages are using the new interface, and this will involve a big change. So we didn't include it in #82477.

ZhouXing19 · 2022-08-17T21:18:55Z

I don't think the current simple tree.CanModifySchema(parsed.AST) && txn != nil check will work. I changed to check if the internal executor is bound to an explicit txn. But in this case, this check is only effective with IE inited via the new interfaces.

ajwerner · 2022-08-17T21:45:32Z

Can you say more about why the check does not work?

ajwerner · 2022-08-17T21:47:25Z

It feels like we need it to be the case that:

If you use the new interface and the user passes a different transaction, that should definitely be an error. I don't know what we should do if the user passes nil, but my instinct is that for now that should also be an error
If you try to do a DDL and you didn't construct the transaction in the right way with a bound collection etc, that should be an error.

ZhouXing19 · 2022-08-17T21:56:25Z

Can you say more about why the check does not work?

Maybe I gave the conclusion too fast -- it was because I saw it broke a great number of tests in the CI. But they were broken because it couldn't run create-file-table, and clean up temporary type. Maybe we should just pass nil txn for them?

It feels like we need it to be the case that ...

Yeah, it makes sense, will add them as well. (Though I have a stronger feeling that the new interface makes everything messier now 😥)

ZhouXing19 · 2022-08-18T17:22:01Z

If you try to do a DDL and you didn't construct the transaction in the right way with a bound collection etc, that should be an error.

If we want to apply this restriction, I think we still need to find out all existing DDL executions with not-nil txn, and let them either run with collectionFactory.TxnWithExeutor() or with nil txn. It turns out that except for those listed in #76764, there're more of them. I'm trying to change them to use nil txn for now and see what will happen ...

ZhouXing19 · 2022-08-19T16:34:21Z

I've changed this PR to refactor the DDL use cases with descs.TxnWithExecutor(), which is broken and relies on #86427. This PR may also overlap with #86433, so I'll rebase from these 2 afterward.

ZhouXing19 · 2022-09-08T20:21:46Z

Sorry for the noise, was fixing tiny nits in the CI.
The PR is now ready for another look.

pkg/sql/create_external_connection.go

pkg/sql/discard.go

pkg/sql/internal.go

ajwerner

LGTM. All my comments are minor nits

rafiss

very nice!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @shermanCRL, and @ZhouXing19)

pkg/ccl/backupccl/backup_test.go line 8183 at r10 (raw file):

		blobs.TestBlobServiceClient(dir),
		username.RootUserName(),
		nil, /* Internal Executor */

nit: the convention here (and for all other inline argument comments) would be to use the name of the parameter, but this isn't a huge deal

This is part of the migration of existing DDL statement with internal executor to `descs.CollectionFactory.TxnWithExecutor()`. DDL statements should only be run with an internal executor that's created via this function. Release justification: Low risk, high benefit changes to existing functionality Release note: none

fixes cockroachdb#76764 Release justification: Low risk, high benefit changes to existing functionality Release note: none

…r()` for DDLs This is part of the project to migrate existing DDL statements running with an internal executor to `descs.CollectionFactory()`. DDLs are only allowed to run with internal executor inited via this function. Release justification: Low risk, high benefit changes to existing functionality Release note: None

…tor()` for DDLs This commit is to migrate the existing DDLs to using `planner.WithInternalExecutor()`. DDLs with internal executors are only allowed if the latter is bounded with txn-realated metadata. Release justification: Low risk, high benefit changes to existing functionality Release note: none

…tor()` for DDLs This commit is part of the project to migrate DDLs running with internal executor with the correct interface. DDLs are only allowed to run with internal executor that is bound to txn-related metadata. Release justification: Low risk, high benefit changes to existing functionality Release note: none

We stripped `txn` from the parameter list in `cleanupTempSystemTables()`. It was run with not-nil txn by mistake, which is a mis-usage of running internal executor with DDLs. Release justification: bug fix Release note: none

This is another DDL statement executed via an internal executor mal-inited. Change it to use the right interface. Release justification: Release note: none.

…tor with txn When using internal executor to run DDL statements under a not-nil outer txn, we require txn-related metadata (such as descriptor collections) to be passed to the internal executor from the outer caller too. This commit is to add a gate for this use case. Release justification: bug fix Release note: none

It was causing the lint in CI to fail. Release note: None

ZhouXing19 · 2022-09-20T15:12:44Z

pkg/ccl/backupccl/backup_test.go line 8183 at r10 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: the convention here (and for all other inline argument comments) would be to use the name of the parameter, but this isn't a huge deal

Done, thanks!

ZhouXing19 · 2022-09-20T16:35:12Z

TFTR!
The CI test failure seems flaky.
bors r=ajwerner,rafiss

craig · 2022-09-20T17:20:39Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-09-20T19:59:22Z

Build succeeded:

Bazel Essential CI (Cockroach)

ZhouXing19 force-pushed the disallow-ddl-0817 branch from 217820f to ebb6736 Compare August 17, 2022 21:16

ZhouXing19 force-pushed the disallow-ddl-0817 branch from ebb6736 to 1325c82 Compare August 18, 2022 21:56

ZhouXing19 force-pushed the disallow-ddl-0817 branch from 1325c82 to 7e91991 Compare August 24, 2022 15:41

ZhouXing19 changed the title ~~sql: disallow DDL statement for internal executor if given a transaction~~ sql: add restriction to run DDL with internal executors Aug 24, 2022

ZhouXing19 marked this pull request as ready for review August 24, 2022 16:57

ZhouXing19 requested review from a team as code owners August 24, 2022 16:57

ZhouXing19 requested a review from a team August 24, 2022 16:57

ZhouXing19 requested review from a team as code owners August 24, 2022 16:57

ZhouXing19 requested a review from a team August 24, 2022 16:57

ZhouXing19 requested review from a team as code owners August 24, 2022 16:57

ZhouXing19 requested review from a team, dt, shermanCRL, ajwerner and rafiss and removed request for a team August 24, 2022 16:57

ZhouXing19 changed the title ~~sql: add restriction to run DDL with internal executors~~ sql: add restriction to running DDL with internal executors Aug 24, 2022

ZhouXing19 force-pushed the disallow-ddl-0817 branch from b1057d9 to 24440da Compare September 8, 2022 20:20

ajwerner reviewed Sep 19, 2022

View reviewed changes

pkg/sql/create_external_connection.go Outdated Show resolved Hide resolved

ajwerner reviewed Sep 19, 2022

View reviewed changes

pkg/sql/discard.go Outdated Show resolved Hide resolved

ajwerner reviewed Sep 19, 2022

View reviewed changes

pkg/sql/internal.go Outdated Show resolved Hide resolved

ajwerner reviewed Sep 19, 2022

View reviewed changes

pkg/sql/internal.go Outdated Show resolved Hide resolved

ajwerner approved these changes Sep 19, 2022

View reviewed changes

ZhouXing19 force-pushed the disallow-ddl-0817 branch from 24440da to 6fa85ec Compare September 19, 2022 19:54

ZhouXing19 requested a review from a team as a code owner September 19, 2022 19:54

ZhouXing19 changed the title ~~sql: add restriction to running DDL with internal executors~~ *: add restriction to running DDL with internal executors Sep 19, 2022

rafiss approved these changes Sep 20, 2022

View reviewed changes

ZhouXing19 added 9 commits September 20, 2022 10:10

filetable: use CollectionFactory.TxnWithExecutor() for DDL statements

3c44cad

fixes cockroachdb#76764 Release justification: Low risk, high benefit changes to existing functionality Release note: none

backupccl: refactor cleanupTempSystemTables()

e8a4d50

We stripped `txn` from the parameter list in `cleanupTempSystemTables()`. It was run with not-nil txn by mistake, which is a mis-usage of running internal executor with DDLs. Release justification: bug fix Release note: none

sql: migrate deleteTempTables to use planner.WithInternalExecutor()

b458abe

This is another DDL statement executed via an internal executor mal-inited. Change it to use the right interface. Release justification: Release note: none.

sql: add a space to a line to fix the lint

ebd6076

It was causing the lint in CI to fail. Release note: None

ZhouXing19 force-pushed the disallow-ddl-0817 branch from 6fa85ec to ebd6076 Compare September 20, 2022 15:10

shermanCRL removed their request for review September 20, 2022 15:22

craig bot merged commit 4d66931 into cockroachdb:master Sep 20, 2022

blathers-crl bot mentioned this pull request Sep 20, 2022

release-22.2: *: add restriction to running DDL with internal executors #88287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add restriction to running DDL with internal executors #86334

*: add restriction to running DDL with internal executors #86334

ZhouXing19 commented Aug 17, 2022 •

edited

Loading

cockroach-teamcity commented Aug 17, 2022

ajwerner commented Aug 17, 2022

ZhouXing19 commented Aug 17, 2022 •

edited

Loading

ZhouXing19 commented Aug 17, 2022

ajwerner commented Aug 17, 2022

ajwerner commented Aug 17, 2022

ZhouXing19 commented Aug 17, 2022

ZhouXing19 commented Aug 18, 2022 •

edited

Loading

ZhouXing19 commented Aug 19, 2022

ZhouXing19 commented Sep 8, 2022

ajwerner left a comment

rafiss left a comment

ZhouXing19 commented Sep 20, 2022

ZhouXing19 commented Sep 20, 2022

craig bot commented Sep 20, 2022

craig bot commented Sep 20, 2022

*: add restriction to running DDL with internal executors #86334

*: add restriction to running DDL with internal executors #86334

Conversation

ZhouXing19 commented Aug 17, 2022 • edited Loading

cockroach-teamcity commented Aug 17, 2022

ajwerner commented Aug 17, 2022

ZhouXing19 commented Aug 17, 2022 • edited Loading

ZhouXing19 commented Aug 17, 2022

ajwerner commented Aug 17, 2022

ajwerner commented Aug 17, 2022

ZhouXing19 commented Aug 17, 2022

ZhouXing19 commented Aug 18, 2022 • edited Loading

ZhouXing19 commented Aug 19, 2022

ZhouXing19 commented Sep 8, 2022

ajwerner left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ZhouXing19 commented Sep 20, 2022

ZhouXing19 commented Sep 20, 2022

craig bot commented Sep 20, 2022

craig bot commented Sep 20, 2022

ZhouXing19 commented Aug 17, 2022 •

edited

Loading

ZhouXing19 commented Aug 17, 2022 •

edited

Loading

ZhouXing19 commented Aug 18, 2022 •

edited

Loading