Skip to content

CDRIVER-4110: Default for causal_consistency depends on snapshot #838

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 4, 2021
Merged

CDRIVER-4110: Default for causal_consistency depends on snapshot #838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0142e80
Don't force casualConsistency=false when creating session with snapsh…
jmikola Jul 30, 2021
47b67d2
Relocate session snapshot prose test
jmikola Jul 30, 2021
18df1e1
Default for causal_consistency depends on snapshot
jmikola Jul 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions src/libmongoc/src/mongoc/mongoc-client-session-private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,9 @@ struct _mongoc_transaction_opt_t {
int64_t max_commit_time_ms;
};

typedef enum {
MONGOC_SESSION_NO_OPTS = 0,
MONGOC_SESSION_CAUSAL_CONSISTENCY = (1 << 0),
MONGOC_SESSION_SNAPSHOT = (2 << 0),
} mongoc_session_flag_t;

struct _mongoc_session_opt_t {
mongoc_session_flag_t flags;
mongoc_optional_t causal_consistency;
mongoc_optional_t snapshot;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems better to have these as optionals instead of on the same flag, thanks. Should the type be mongoc_optional_bool_t or is that type name change API-breaking?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I brought that up with @kevinAlbs in Slack last week and we concluded that the ship has sailed since mongoc_optional_t is already in the public API (shipped with Versioned API in 1.18.0). If we end up introducing an optional int type down the line, perhaps we can deprecate the original name and introduce mongoc_optional_bool_t alongside mongoc_optional_int_t, but I don't think there's anything to do presently.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @jmikola!

Here is my understanding.

Before snapshot reads:

  • mongoc_session_opts_new sets causalConsistency=true.

After snapshot reads:

  • mongoc_session_opts_new sets causalConsistency=true and snapshot=false.
  • Validation occurs in mongoc_client_start_session (i.e. both being true is an error).

The new behavior proposed in this PR:

  • mongoc_session_opts_new sets causalConsistency=unset and snapshot=unset
  • Defaults are applied in mongoc_client_start_session if the values are unset. Validation occurs in mongoc_client_start_session.

Admittedly, when reviewing the snapshot reads changes, I had thought the behavior was correct since it built off of the previous behavior. I had not considered introducing an unset value for causalConsistency and snapshot. This seems correct.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your understanding is correct.

I think much of the confusion actually relates in some way to SPEC-936, which added the technical equivalent of legalese to state that the causalConsistency session option doesn't get a default value (which would require modeling it as a trilean), but instead inherits some default. In practice, I think some drivers (libmongoc included) just defaulted it to true as that had a functionally equivalent effect.

mongoc_transaction_opt_t default_txn_opts;
};

Expand Down
41 changes: 19 additions & 22 deletions src/libmongoc/src/mongoc/mongoc-client-session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,14 @@ mongoc_session_opts_get_causal_consistency (const mongoc_session_opt_t *opts)

BSON_ASSERT (opts);

RETURN (!!(opts->flags & MONGOC_SESSION_CAUSAL_CONSISTENCY));
/* Causal Consistency spec: If no value is provided for causalConsistency
* and snapshot reads are not requested a value of true is implied. */
if (!mongoc_optional_is_set (&opts->causal_consistency) &&
!mongoc_optional_value (&opts->snapshot)) {
RETURN (true);
}

RETURN (mongoc_optional_value (&opts->causal_consistency));
}

bool
Expand All @@ -413,7 +420,7 @@ mongoc_session_opts_get_snapshot (const mongoc_session_opt_t *opts)

BSON_ASSERT (opts);

RETURN (!!(opts->flags & MONGOC_SESSION_SNAPSHOT));
RETURN (mongoc_optional_value (&opts->snapshot));
}

void
Expand All @@ -424,11 +431,7 @@ mongoc_session_opts_set_causal_consistency (mongoc_session_opt_t *opts,

BSON_ASSERT (opts);

if (causal_consistency) {
opts->flags |= MONGOC_SESSION_CAUSAL_CONSISTENCY;
} else {
opts->flags &= ~MONGOC_SESSION_CAUSAL_CONSISTENCY;
}
mongoc_optional_set_value (&opts->causal_consistency, causal_consistency);

EXIT;
}
Expand All @@ -440,11 +443,7 @@ mongoc_session_opts_set_snapshot (mongoc_session_opt_t *opts, bool snapshot)

BSON_ASSERT (opts);

if (snapshot) {
opts->flags |= MONGOC_SESSION_SNAPSHOT;
} else {
opts->flags &= ~MONGOC_SESSION_SNAPSHOT;
}
mongoc_optional_set_value (&opts->snapshot, snapshot);

EXIT;
}
Expand All @@ -454,11 +453,8 @@ mongoc_session_opts_new (void)
{
mongoc_session_opt_t *opts = bson_malloc0 (sizeof (mongoc_session_opt_t));

/* Driver Sessions Spec: causal consistency is true by default */
mongoc_session_opts_set_causal_consistency (opts, true);

/* Snapshot Reads Spec: snapshot is false by default */
mongoc_session_opts_set_snapshot (opts, false);
mongoc_optional_init (&opts->causal_consistency);
mongoc_optional_init (&opts->snapshot);

return opts;
}
Expand Down Expand Up @@ -513,7 +509,8 @@ static void
_mongoc_session_opts_copy (const mongoc_session_opt_t *src,
mongoc_session_opt_t *dst)
{
dst->flags = src->flags;
mongoc_optional_copy (&src->causal_consistency, &dst->causal_consistency);
mongoc_optional_copy (&src->snapshot, &dst->snapshot);
txn_opts_copy (&src->default_txn_opts, &dst->default_txn_opts);
}

Expand Down Expand Up @@ -800,22 +797,22 @@ _mongoc_client_session_new (mongoc_client_t *client,
session->client_session_id = client_session_id;
bson_init (&session->cluster_time);

mongoc_optional_init (&session->opts.causal_consistency);
mongoc_optional_init (&session->opts.snapshot);
txn_opts_set (&session->opts.default_txn_opts,
client->read_concern,
client->write_concern,
client->read_prefs,
DEFAULT_MAX_COMMIT_TIME_MS);

if (opts) {
session->opts.flags = opts->flags;
mongoc_optional_copy (&opts->causal_consistency, &session->opts.causal_consistency);
mongoc_optional_copy (&opts->snapshot, &session->opts.snapshot);
txn_opts_set (&session->opts.default_txn_opts,
opts->default_txn_opts.read_concern,
opts->default_txn_opts.write_concern,
opts->default_txn_opts.read_prefs,
opts->default_txn_opts.max_commit_time_ms);
} else {
/* sessions are causally consistent by default */
session->opts.flags = MONGOC_SESSION_CAUSAL_CONSISTENCY;
}

/* snapshot_time_set is false by default */
Expand Down
67 changes: 66 additions & 1 deletion src/libmongoc/tests/test-mongoc-client-session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ test_session_opts_clone (void)

opts = mongoc_session_opts_new ();
clone = mongoc_session_opts_clone (opts);
/* causal is enabled by default */
/* causalConsistency is enabled by default if snapshot is not enabled */
BSON_ASSERT (mongoc_session_opts_get_causal_consistency (clone));
mongoc_session_opts_destroy (clone);

Expand All @@ -47,6 +47,31 @@ test_session_opts_clone (void)
}


static void
test_session_opts_causal_consistency_and_snapshot (void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this test, we now have a similar one in the Go driver.

{
mongoc_session_opt_t *opts;

opts = mongoc_session_opts_new ();
/* causalConsistency is enabled by default if snapshot is not enabled */
BSON_ASSERT (mongoc_session_opts_get_causal_consistency (opts));
BSON_ASSERT (!mongoc_session_opts_get_snapshot (opts));

/* causalConsistency is disabled by default if snapshot is enabled */
mongoc_session_opts_set_snapshot (opts, true);
BSON_ASSERT (!mongoc_session_opts_get_causal_consistency (opts));
BSON_ASSERT (mongoc_session_opts_get_snapshot (opts));

/* causalConsistency and snapshot can both be enabled, although this will
* result in an error when starting the session. */
mongoc_session_opts_set_causal_consistency (opts, true);
BSON_ASSERT (mongoc_session_opts_get_causal_consistency (opts));
BSON_ASSERT (mongoc_session_opts_get_snapshot (opts));

mongoc_session_opts_destroy (opts);
}


static void
test_session_no_crypto (void *ctx)
{
Expand Down Expand Up @@ -2707,12 +2732,44 @@ test_session_dirty (void *unused)
_test_session_dirty_helper (false /* retry succceeds */);
}

void
test_sessions_snapshot_prose_test_1 (void *ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for moving this test.

{
mongoc_client_t *client = NULL;
mongoc_session_opt_t *session_opts = NULL;
bson_error_t error;
bool r;

client = test_framework_new_default_client ();
BSON_ASSERT (client);

session_opts = mongoc_session_opts_new ();
mongoc_session_opts_set_causal_consistency (session_opts, true);
mongoc_session_opts_set_snapshot (session_opts, true);

/* assert that starting session with causal consistency and snapshot enabled
* results in an error. */
r = mongoc_client_start_session (client, session_opts, &error);
ASSERT (!r);
ASSERT_ERROR_CONTAINS (
error,
MONGOC_ERROR_CLIENT,
MONGOC_ERROR_CLIENT_SESSION_FAILURE,
"Only one of causal consistency and snapshot can be enabled.");

mongoc_session_opts_destroy (session_opts);
mongoc_client_destroy (client);
}

void
test_session_install (TestSuite *suite)
{
char resolved[PATH_MAX];

TestSuite_Add (suite, "/Session/opts/clone", test_session_opts_clone);
TestSuite_Add (suite,
"/Session/opts/causal_consistency_and_snapshot",
test_session_opts_causal_consistency_and_snapshot);
TestSuite_AddFull (suite,
"/Session/no_crypto",
test_session_no_crypto,
Expand Down Expand Up @@ -3079,4 +3136,12 @@ test_session_install (TestSuite *suite)
test_framework_skip_if_no_failpoint,
/* Tests with retryable writes, requires non-standalone. */
test_framework_skip_if_single);

TestSuite_AddFull (suite,
"/Session/snapshot/prose_test_1",
test_sessions_snapshot_prose_test_1,
NULL,
NULL,
test_framework_skip_if_no_sessions,
test_framework_skip_if_no_crypto);
}
35 changes: 0 additions & 35 deletions src/libmongoc/tests/test-mongoc-transactions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1166,35 +1166,6 @@ test_max_commit_time_ms_is_reset (void *ctx)
mock_rs_destroy (rs);
}

void
test_snapshot_session_prose_1 (void *ctx)
{
mongoc_client_t *client = NULL;
mongoc_session_opt_t *session_opts = NULL;
bson_error_t error;
bool r;

client = test_framework_new_default_client ();
BSON_ASSERT (client);

session_opts = mongoc_session_opts_new ();
mongoc_session_opts_set_causal_consistency (session_opts, true);
mongoc_session_opts_set_snapshot (session_opts, true);

/* assert that starting session with causal consistency and snapshot enabled
* results in an error. */
r = mongoc_client_start_session (client, session_opts, &error);
ASSERT (!r);
ASSERT_ERROR_CONTAINS (
error,
MONGOC_ERROR_CLIENT,
MONGOC_ERROR_CLIENT_SESSION_FAILURE,
"Only one of causal consistency and snapshot can be enabled.");

mongoc_session_opts_destroy (session_opts);
mongoc_client_destroy (client);
}

void
test_transactions_install (TestSuite *suite)
{
Expand Down Expand Up @@ -1286,10 +1257,4 @@ test_transactions_install (TestSuite *suite)
NULL,
NULL,
test_framework_skip_if_no_crypto);
TestSuite_AddFull (suite,
"/transactions/snapshot_sessions_prose_1",
test_snapshot_session_prose_1,
NULL,
NULL,
test_framework_skip_if_no_txns);
}
2 changes: 0 additions & 2 deletions src/libmongoc/tests/unified/entity-map.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,8 +587,6 @@ session_opts_new (bson_t *bson, bson_error_t *error)
mongoc_session_opts_set_causal_consistency (opts, *causal_consistency);
}
if (snapshot) {
/* Set causal consistency to false to allow snapshot */
mongoc_session_opts_set_causal_consistency (opts, false);
mongoc_session_opts_set_snapshot (opts, *snapshot);
}

Expand Down