googleapis / google-cloud-cpp

C++ Client Libraries for Google Cloud Services
https://cloud.google.com/
Apache License 2.0
554 stars 373 forks source link

Implement non-idempotent Commit (write at least once). #4890

Closed mr-salty closed 1 year ago

mr-salty commented 4 years ago

For earlier context see https://github.com/googleapis/google-cloud-cpp-spanner/issues/689

The summary is that we decided that our default Commit implementation should explicitly call BeginTransaction if no transaction was started, to avoid potentially surprising behavior that occurs if we inline transaction creation with Commit. However, there may be cases where the user wants that behavior (since it avoids an extra RPC), so we should expose a method to do that.

mr-salty commented 4 years ago

Java calls this method writeAtLeastOnce with the following description:

  /**
   * Writes the given mutations atomically to the database without replay protection.
   *
   * <p>Since this method does not feature replay protection, it may attempt to apply {@code
   * mutations} more than once; if the mutations are not idempotent, this may lead to a failure
   * being reported when the mutation was applied once. For example, an insert may fail with {@link
   * ErrorCode#ALREADY_EXISTS} even though the row did not exist before this method was called. For
   * this reason, most users of the library will prefer to use {@link #write(Iterable)} instead.
   * However, {@code writeAtLeastOnce()} requires only a single RPC, whereas {@code write()}
   * requires two RPCs (one of which may be performed in advance), and so this method may be
   * appropriate for latency sensitive and/or high throughput blind writing.
   *
   * <p>Example of unprotected blind write.
   *
   * <pre>{@code
   * long singerId = my_singer_id;
   * Mutation mutation = Mutation.newInsertBuilder("Singers")
   *         .set("SingerId")
   *         .to(singerId)
   *         .set("FirstName")
   *         .to("Billy")
   *         .set("LastName")
   *         .to("Joel")
   *         .build();
   * dbClient.writeAtLeastOnce(Collections.singletonList(mutation));
   * }</pre>
   *
   * @return the timestamp at which the write was committed
   */
devjgm commented 2 years ago

Punting until later.

coryan commented 2 years ago

Second strike.

coryan commented 1 year ago

Assigning to @devbww for analysis. Close it if it does not make sense.

devbww commented 1 year ago

Triaged: I can see that this makes sense, but will keep writeAtLeastOnce() on the back burner as it is still only an internal proposal (and just an optimization).

In the shorter term, though, I do want to address the use of a single-use transaction (non-idempotent) when Commit() is called without having previously begun a transaction. As was previously noted, whether another RPC has been made within the mutator or not shouldn't change the semantics.

devbww commented 1 year ago

In the shorter term, though, I do want to address the use of a single-use transaction (non-idempotent) when Commit() is called without having previously begun a transaction.

This is a non-issue. The (public) API does not allow calling Commit() with a single-use, read-only transaction, so the !id && !begin case cannot happen. But even if it did, the Commit() would fail in the same way it does with any other read-only transaction ... FAILED_PRECONDITION: Cannot commit a read-only transaction.