PHPC-1140: Implement Transactions specification #850

derickr · 2018-06-14T16:25:40Z

No description provided.

jmikola · 2018-06-15T20:52:04Z

src/MongoDB/Manager.c

@@ -669,11 +670,46 @@ static PHP_METHOD(Manager, startSession)
 		return;
 	}

-	if (options && php_array_exists(options, "causalConsistency")) {
+	if (options && php_array_existsc(options, "causalConsistency")) {


IIRC this ensures we use sizeof() instead of strlen(). Good catch 👍

jmikola · 2018-06-15T22:28:18Z

.travis.yml

@@ -32,6 +32,9 @@ matrix:
    - php: 7.0
      env:
        - SERVER_VERSION=3.4.11
+    - php: 7.0
+      env:
+        - SERVER_VERSION=4.0.0-rc4


Relevant: #851

I'm not sure this will be helpful for the transaction tests (pending PHPC-1184), since they require a replica set oplog to exist.

Yeah, I am aware of that. But some tests just need a 4.0.0 one to run at, as they don't actually talk to the Database yet. But libmongoc still does the version check with a single server.

jmikola · 2018-06-15T22:32:01Z

src/MongoDB/Manager.c

+			if (cs_opts) {
+				mongoc_session_opts_destroy(cs_opts);
+			}
+			return;


If we restructure the end of this function, can we utilize a cleanup: label? Consider:

cs = mongoc_client_start_session(intern->client, cs_opts, &error); if (cs) { phongo_session_init(return_value, cs TSRMLS_CC); } else { phongo_throw_exception_from_bson_error_t(&error TSRMLS_CC); } cleanup: if (cs_opts) { mongoc_session_opts_destroy(cs_opts); } }

Then we can use goto cleanup; here and within the if (EG(exception)) block below.

Yes, and a little more too.

jmikola · 2018-06-15T22:36:08Z

src/MongoDB/Manager.c

+
+		/* Thrown exception and return if the defaultTransactionOptions is not an array */
+		if (!txn_options) {
+			phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"defaultTransactionOptions\" option to be an array");


For consistency with other unexpected type exceptions, can we change php_array_fetch_array above to php_array_fetchc and then use the following here?

if (Z_TYPE_P(txn_options) != IS_ARRAY) { phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"defaultTransactionOptions\" option to be array, %s given", PHONGO_ZVAL_CLASS_OR_TYPE_NAME_P(txn_options)); }

jmikola · 2018-06-15T22:36:57Z

src/MongoDB/Manager.c

 		cs_opts = mongoc_session_opts_new();
 		mongoc_session_opts_set_causal_consistency(cs_opts, php_array_fetchc_bool(options, "causalConsistency"));
 	}

+	if (options && php_array_existsc(options, "defaultTransactionOptions")) {
+		zval* txn_options = php_array_fetch_array(options, "defaultTransactionOptions");


Did you mean to use php_array_fetchc_array here? Though, see my comment below about restructuring before you address that.

No longer relevant due to the previous comment.

jmikola · 2018-06-15T22:38:34Z

src/MongoDB/Session.c

@@ -237,6 +385,10 @@ ZEND_BEGIN_ARG_INFO_EX(ai_Session_advanceOperationTime, 0, 0, 1)
 	ZEND_ARG_INFO(0, timestamp)
 ZEND_END_ARG_INFO()

+ZEND_BEGIN_ARG_INFO_EX(ai_Session_startTransAction, 0, 0, 0)


I don't think the "A" should be capitalized: "startTransaction"

Good catch.

jmikola · 2018-06-15T23:02:08Z

src/MongoDB/Session.c

+		return;
+	}
+
+	if (!mongoc_client_session_commit_transaction(intern->client_session, NULL, &error)) {


Is there any value in capturing the reply to commitTransaction here?

txn_finish() uses mongoc_client_write_command_with_opts() to execute the command, so we may want to check the error domain and conditionally upgrade a ServerException into a CommandException and stash the reply on it, as we do from executeCommand() and friends.

I think so, but as we're then doing this now in multiple places, we probably should create a helper method akin to phongo_throw_exception_from_bson_error_t — for example phongo_throw_exception_from_bson_error_and_reply ?

jmikola · 2018-06-15T23:12:27Z

tests/session/session-startTransaction-001.phpt

+MongoDB\Driver\Session::startTransaction() mini test
+--SKIPIF--
+<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
+<?php skip_if_server_version('<', '4.0'); ?>


AFAIK, transactions require a replica set oplog. You should probably add skip_if_not_replica_set() here as well.

All session-related stuff requires libmongoc crypto as well, so skip_if_not_libmongoc_crypto() should appear as the first "skip if" (for consistency). I believe all of the other session tests have that.

Should this

Happy to add the crypto stuff, but why do sessions require it?

I believe it's used for generating a session ID.

jmikola · 2018-06-15T23:17:55Z

tests/session/session-startTransaction-001.phpt

+$session->abortTransaction();
+
+$session->startTransaction();
+$session->commitTransaction();


I tested these methods against a 4.0.0-rc5 standalone and there were no errors in any of the transaction tests. I found that a bit odd as transactions require a replica set oplog.

With regard to this test alone (I haven't reviewed the others yet), that may be due to the fact that a transaction is not actually started until the first command executed within in. See startTransaction in the spec. Therefore, I would propose we do an insertOne or some other basic write within each transaction (between start and abort/commit).

Additionally, we may want another test to demonstrate an error when executing on a 4.0+ standalone (there are appropriate "skip_if" functions for that, too).

Replying to this and the above comment. This test does not require a working replicaset as it doesn't actually run a transaction. This test tests whether I could call the methods at all. libmongoc doesn't check whether there is a replica set (that's a server error), but it does test whether the server has the right wire protocol version (hence the < 4.0 check).

Noted. I'd suggest a comment to explain that. The title only said "mini test" so it wasn't clear what the intention was.

Even something like "ensure that methods can be called" as the title would add some more clarity.

jmikola · 2018-06-18T14:35:13Z

src/MongoDB/Manager.c

+	if (cs_opts) {
+		mongoc_session_opts_destroy(cs_opts);
+	}
+	if (txn_opts) {


Ah, were these never freed before? I didn't notice any memory leaks.

Nope, they weren't it seems. I didn't see any leaks either though.

I think https://github.com/mongodb/mongo-php-driver/pull/850/files#r196198256 highlights a possible leak that I may have been thinking off, since that's one case where php_mongodb_session_parse_transaction_options() could return 0 while still having allocated memory.

Once that leak is addressed, I don't think we need this in cleanup: as we can call mongoc_transaction_opts_destroy() immediately after setting it on the session options.

jmikola · 2018-06-18T14:38:12Z

src/MongoDB/Session.c

+	}
+
+	if (!mongoc_client_session_commit_transaction(intern->client_session, &reply, &error)) {
+		phongo_throw_exception_from_bson_error_and_reply_t(&error, &reply TSRMLS_CC);


Test this for leaks. I think you still need a bson_destroy(&reply); after this.

executeCommand() does so before returning as its free_reply boolean will be true when it calls phongo_throw_exception_from_bson_error_and_reply_t.

Ah, you're right — fixing.

jmikola · 2018-06-18T16:15:06Z

scripts/presets/replicaset-30.json

@@ -12,6 +12,7 @@
 				"noprealloc": true,
 				"nssize": 1,
 				"port": 3100,
+				"bind_ip_all": true,


bind_ip_all was introduced in 3.6, so I don't think we should specify it for the configs using "30-release".

That said, I do think we should add it to all of the non-versioned configs, which will now be using "40-release". This includes all of the standalone JSON files apart from standalone-30.json.

I suppose you can do "bind_ip": "0.0.0.0,::" for the 3.0 configs, per the documentation above.

jmikola · 2018-06-18T16:16:23Z

scripts/ubuntu/mongo-orchestration-config.json

@@ -1,5 +1,6 @@
 {
 	"releases": {
+		"40-release": "/home/vagrant/4.0/usr/bin",
 		"36-release": "/home/vagrant/3.6/usr/bin",


I don't think anything is using this. It was only important as the first version listed under "releases", which made it the default. Now that "40-release" is here, I think we can just remove this.

No objections if you want to keep it around, though. scripts/ubuntu/mongo-orchestration.sh is still installing it, so there's no harm in leaving this here.

We'll leave it for now.

jmikola · 2018-06-18T16:19:38Z

tests/utils/tools.php

@@ -45,7 +45,7 @@ function drop_collection($uri, $databaseName, $collectionName)
    $command = new Command(['drop' => $collectionName]);

    try {
-        $server->executeCommand($databaseName, $command);
+        $server->executeCommand($databaseName, $command, [ 'writeConcern' => new \MongoDB\Driver\WriteConcern( \MongoDB\Driver\WriteConcern::MAJORITY ) ] );


Why was this necessary? Did it interfere with tests that query the secondary and expected an empty collection?

No: https://jira.mongodb.org/browse/SERVER-35613 — I need to come up with whether I think this is Okay behaviour in the server though. It's certainly going to be mega confusing for users.

The explanation makes sense, but this is indeed very weird. Can we add a comment above this line explaining the point of the write concern, along with "see: SERVER-35613"?

Reminder to add a comment and reference to SERVER-35613.

jmikola · 2018-06-18T16:21:29Z

tests/session/session-startTransaction_error-003.phpt

+===DONE===
+<?php exit(0); ?>
+--EXPECTF--
+Warning: MongoDB\Driver\Session::startTransaction() expects parameter 1 to be array, integer given in %s%session-startTransaction_error-003.php on line %d


Other tests simply use in %s on line %d. Any reason not to do the same here and make the output a bit more robust?

ah, crap. That's what you get doing it different in your other major (Xdebug) project. Will fix (again).

jmikola · 2018-06-18T19:27:45Z

src/MongoDB/Manager.c

-		mongoc_session_opts_destroy(cs_opts);
+		/* Thrown exception and return if the defaultTransactionOptions is not an array */
+		if (Z_TYPE_P(txn_options) != IS_ARRAY) {
+			phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"defaultTransactionOptions\" option to be an array");


I think my original request in #850 (comment) was lost by subsequent edits. Can we add ", %s given" for consistency with other exceptions? The inconsistency stands out in the error tests you have below, which is also how I realized this change wasn't made.

jmikola · 2018-06-18T19:30:13Z

src/MongoDB/Session.c

+
+		if (Z_TYPE_P(read_concern) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(read_concern), php_phongo_readconcern_ce TSRMLS_CC)) {
+			phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"readConcern\" option to be %s, %s given", ZSTR_VAL(php_phongo_readconcern_ce->name), PHONGO_ZVAL_CLASS_OR_TYPE_NAME_P(read_concern));
+			return false;


I realize false is 0, but this should technically return NULL. As-is, one might read this and assume the function returns a success boolean.

jmikola · 2018-06-18T19:33:06Z

src/MongoDB/Session.c

+
+		if (Z_TYPE_P(write_concern) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(write_concern), php_phongo_writeconcern_ce TSRMLS_CC)) {
+			phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"writeConcern\" option to be %s, %s given", ZSTR_VAL(php_phongo_writeconcern_ce->name), PHONGO_ZVAL_CLASS_OR_TYPE_NAME_P(write_concern));
+			return false;


Technically, it may be possible for this to leak if a valid ReadConcern or ReadPreference is added to mongoc_transaction_opt_t* opts but the WriteConcern fails. Likewise for a valid ReadConcern and invalid ReadPreference. Both of these cases should be easy to test and demonstrate a leak. I imagine the current error tests didn't expose this because the test cases each contain one option at a time.

Should just require adding the following here and in ReadPreference before what should be return NULL (per my comment above):

if (opts) { mongoc_transaction_opts_destroy(opts); }

jmikola · 2018-06-18T19:38:36Z

tests/session/session-startTransaction_error-002.phpt

+    [ 'readPreference' => new \MongoDB\Driver\ReadConcern( \MongoDB\Driver\ReadConcern::LOCAL ) ],
+    [ 'writeConcern' => 42 ], 
+    [ 'writeConcern' => new stdClass ],
+    [ 'writeConcern' => new \MongoDB\Driver\ReadPreference( \MongoDB\Driver\ReadPreference::RP_SECONDARY ) ],


I'd suggest two or three new cases:

Valid ReadConcern and invalid ReadPreference

Valid ReadConcern and invalid WriteConcern (optional)

Valid ReadPreference and invalid WriteConcern

This would test the memory leak I noted above.

jmikola · 2018-06-18T19:39:34Z

tests/session/session_error-001.phpt

+    [ 'readPreference' => new \MongoDB\Driver\ReadConcern( \MongoDB\Driver\ReadConcern::LOCAL ) ],
+    [ 'writeConcern' => 42 ], 
+    [ 'writeConcern' => new stdClass ],
+    [ 'writeConcern' => new \MongoDB\Driver\ReadPreference( \MongoDB\Driver\ReadPreference::RP_SECONDARY ) ],


Likewise, this can include test cases I suggested in https://github.com/mongodb/mongo-php-driver/pull/850/files#r196199595 (redundant, but not harm in keeping the tests consistent).

jmikola · 2018-06-18T19:40:34Z

tests/session/transaction-integration-001.phpt

@@ -0,0 +1,82 @@
+--TEST--
+MongoDB\Driver\Session::startTransaction() mini test


Would "Committing a transaction" be a better name? I assume this doesn't encounter any errors and just demonstrates a transaction across two collections.

I've done that. It's basically the same as the example from the PHPLIB requested docs: https://jira.mongodb.org/browse/PHPLIB-350

jmikola · 2018-06-18T19:43:03Z

tests/session/transaction-integration-002.phpt

+
+$manager = new MongoDB\Driver\Manager(URI);
+
+/* Create collections as that can't be (automatically) done in a transaction */


So no implicit creation of collections/databases within a transaction? I wasn't aware of that limitation.

I suppose the create command doesn't have the same issue with drop in that it requires a majority WC, correct?

Correct. create isn't two phase, I believe.

And yes, just like older MySQL's, support for DDL changes in transactions doesn't seem to be included. At least we get an exception instead of silently ignoring it.

jmikola · 2018-06-18T19:47:41Z

src/MongoDB/Manager.c

+				cs_opts = mongoc_session_opts_new();
+			}
+
+			mongoc_session_opts_set_default_transaction_opts(cs_opts, txn_opts);


Can we call mongoc_transaction_opts_destroy() after this? Ideally, this should be the only point where txn_opts is non-NULL and in need of freeing (provided the memory leak in https://github.com/mongodb/mongo-php-driver/pull/850/files#r196198256 is fixed).

Yes, done. And updated relevant comments.

jmikola · 2018-06-19T13:26:27Z

scripts/presets/standalone-30.json

@@ -8,6 +8,7 @@
        "logpath": "/tmp/standalone-30/mongod.log",
        "journal": true,
        "port": 2700,
+        "bind_ip_all": true,


This is a 3.0 server, so you want "bind_ip": "0.0.0.0,::", here.

jmikola · 2018-06-19T13:27:48Z

src/MongoDB/Session.c

+			phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"readConcern\" option to be %s, %s given", ZSTR_VAL(php_phongo_readconcern_ce->name), PHONGO_ZVAL_CLASS_OR_TYPE_NAME_P(read_concern));
+			/* Freeing opts is not needed here, as it can't be set yet. The
+			 * code is here to keep it consistent with the others in case more
+			 * options are added before this one. */


Thanks for the comment 👍

jmikola

Small changes:

Fix binding param for standalone 3.0 config
Two extra line breaks can be removed from phpt files
Add a comment about the drop WC in tools.php

LGTM otherwise.

jmikola · 2018-06-19T13:32:42Z

tests/session/transaction-integration-001.phpt

+--EXPECTF--
+Transaction committed.
+===DONE===
+


Extra line break here?

jmikola · 2018-06-19T13:32:51Z

tests/session/transaction-integration-002.phpt

+--EXPECTF--
+found a TransientTransactionError
+===DONE===
+


Extra line break here?

jmikola · 2018-06-19T13:33:24Z

tests/utils/tools.php

@@ -45,7 +45,7 @@ function drop_collection($uri, $databaseName, $collectionName)
    $command = new Command(['drop' => $collectionName]);

    try {
-        $server->executeCommand($databaseName, $command);
+        $server->executeCommand($databaseName, $command, [ 'writeConcern' => new \MongoDB\Driver\WriteConcern( \MongoDB\Driver\WriteConcern::MAJORITY ) ] );


Reminder to add a comment and reference to SERVER-35613.

derickr force-pushed the PHPC-1140-transactions branch from b467ca4 to 747420b Compare June 15, 2018 09:03

jmikola reviewed Jun 15, 2018

View reviewed changes

derickr force-pushed the PHPC-1140-transactions branch from 1641d81 to 30caa4b Compare June 18, 2018 12:14

Fixed php_array_exists call

96dfb06

derickr force-pushed the PHPC-1140-transactions branch from b8c9ed0 to 446db65 Compare June 18, 2018 14:34

jmikola reviewed Jun 18, 2018

View reviewed changes

Add 4.0 to Vagrant test set-up

0f2e387

derickr force-pushed the PHPC-1140-transactions branch from 370b262 to a4fdc58 Compare June 19, 2018 10:16

jmikola reviewed Jun 19, 2018

View reviewed changes

jmikola approved these changes Jun 19, 2018

View reviewed changes

jmikola changed the title ~~[WIP] PHPC-1140 Implement Transactions specification~~ PHPC-1140 Implement Transactions specification Jun 19, 2018

jmikola changed the title ~~PHPC-1140 Implement Transactions specification~~ PHPC-1140: Implement Transactions specification Jun 19, 2018

PHPC-1140: Implement Transactions specification

b172a20

derickr force-pushed the PHPC-1140-transactions branch from a4fdc58 to b172a20 Compare June 19, 2018 13:47

jmikola approved these changes Jun 19, 2018

View reviewed changes

derickr merged commit b172a20 into mongodb:master Jun 19, 2018

derickr added a commit that referenced this pull request Jun 19, 2018

Merged pull request #850

a207c77

jmikola mentioned this pull request Jun 20, 2018

Improve error reporting in start-servers.php and fix 3.0 servers #856

Merged

		@@ -0,0 +1,82 @@
		--TEST--
		MongoDB\Driver\Session::startTransaction() mini test


		$manager = new MongoDB\Driver\Manager(URI);

		/* Create collections as that can't be (automatically) done in a transaction */

PHPC-1140: Implement Transactions specification #850

PHPC-1140: Implement Transactions specification #850

Conversation

derickr commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Jun 15, 2018 •

edited

Loading

jmikola Jun 18, 2018 •

edited

Loading

jmikola Jun 18, 2018 •

edited

Loading