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

RFC: asynchronous mutations (automatic RETURNING NOTHING) #25230

Closed
wants to merge 1 commit into from

Conversation

knz
Copy link
Contributor

@knz knz commented May 2, 2018

Release note: none

@knz knz requested a review from a team as a code owner May 2, 2018 12:52
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20180502-async-mutations branch from c3bf081 to abba044 Compare May 2, 2018 14:10
@knz knz force-pushed the 20180502-async-mutations branch from abba044 to d9c2881 Compare May 2, 2018 14:14
@bdarnell
Copy link
Contributor

bdarnell commented May 3, 2018

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


docs/RFCS/async_mutations.md, line 49 at r1 (raw file):

NOTHING boggles my mind. Parallelization should be enabled by
default. Then I hear someone say 'wait we can't do that because of SQL
semantics', well, {censored} SQL semantics!"

SQL semantics are inconvenient, but they're still there. We have to return something for the row count. Do we just return 0? 1? Do the ORMs that make it difficult for people to use RETURNING NOTHING also do any validation of the returned row count? (SQLAlchemy does in at least some situations). The SQLAlchemy ORM also tends to add RETURNING clauses to its INSERT statements whenever there is a server-assigned primary key, which reduces the benefit from this proposal. I don't think this is a dealbreaker, but the fact that this setting may be subtly incompatible with some ORMs means that there is extra complexity that we'll need to document.


docs/RFCS/async_mutations.md, line 65 at r1 (raw file):

below). Once that mode is enabled, CockroachDB accepts mutation
statements (INSERT/UPDATE/etc) faster than they are processed, so that
they can be processed in parallel in multiple cores/nodes.

In my mind this is less about parallelism across multiple nodes and more about not waiting for network round trips.


docs/RFCS/async_mutations.md, line 68 at r1 (raw file):

(TODO: check the following paragraph. This may be an opt-in feature.)
When the fast mode is enabled however, a mutation statement may not

I think that the asynchronous mutations setting should have exactly the same semantics as adding RETURNING NOTHING to all insertion statements. We (probably) don't want to introduce a second slightly different mode that has harder-to-understand semantics.


docs/RFCS/async_mutations.md, line 75 at r1 (raw file):

Finally, clients can then either ignore the outcome (the mutations
will be eventually processed), or check for results (which errors have
happened or how many rows were mutated) with the new `WAIT` statement.

Just WAIT with no arguments? I'm wary of claiming a top-level statement for this. Maybe a WAIT FOR MUTATIONS statement to give us the opportunity to wait for other things in the future?

I'm not sure a new wait command is necessary. What's wrong with the current semantics of flushing all async statements at COMMIT/RELEASE SAVEPOINT? WAIT would be a brand-new statement that could not be used without code changes, and I'm skeptical that it would get much use.


docs/RFCS/async_mutations.md, line 86 at r1 (raw file):

the statement stats) to annotate mutations issued asynchronously.

## Overlap with other proposals

What about the pipelining/async writes proposal in #16026? This proposal seems redundant with that one.


docs/RFCS/async_mutations.md, line 122 at r1 (raw file):

- typically reflects the reality of multi-app deployments: some apps
  will want to use the feature, other apps don't want
  to. `application_name` is the canonical discriminant between

I'd consider putting this on the user/role instead. Postgres has an analogous feature; I've not heard of using application_name for this purpose before.

#21151


docs/RFCS/async_mutations.md, line 159 at r1 (raw file):

  blockers.)

- Whether COMMIT would automatically WAIT if the feature is enabled

I think that COMMIT (including implicit single-statement txns) should wait, just like we do today for RETURNING NOTHING. Making COMMIT asynchronous is very different from making mutations inside a txn asynchronous. (Maybe we want to allow that too, but it would be a separate option. Personally I'd generally want to turn on async mutations but leave commits synchronous).


Comments from Reviewable

@knz knz closed this May 20, 2019
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.

3 participants