oci8 - Implementation of Oracle TAF Callback (PHP5) by KoenigsKind · Pull Request #2405 · php/php-src

KoenigsKind · 2017-03-06T14:13:15Z

Adds support for the Transparent Application Failover Callback.
The php_oci_connection struct got a char* added which will contain the callback function, it should be set to PHP_OCI_TAF_DISABLE_CALLBACK at the end of a php request for permanent connections so that, if a TAF callback occurs, no userspace function will be called.
Maybe add support for registering object functions (via array), currently the register function only accepts a string. I didn't know how to implement it correctly. As a failover occurs very rarely it might be better to not keep the cache when saving the zend_fcall_info.

Things to do

config.m4 needs to compile oci8_failover.c
Check if correctly implemented (especially for multithreading)
Add support for object functions

New pull request
Created a new pull request for PHP 7: #2459

Adds support for the Transparent Application Failover Callback. The php_oci_connection struct got a char* added which will contain the callback function, it should be set to PHP_OCI_TAF_DISABLE_CALLBACK at the end of a php request for permanent connections so that, if a TAF callback occurs, no userspace function will be called. Maybe add support for registering object functions (via array), currently the register function only accepts a string. I didn't know how to implement it correctly. As a failover occurs very rarely it might be better to not keep the cache when saving the zend_fcall_info. Things to do [ ] config.m4 needs to compile oci8_failover.c [ ] Check if correctly implemented (especially for multithreading) [ ] Add support for object functions

nikic · 2017-03-06T14:27:48Z

PHP 5.6 is no longer actively supported, so this change cannot go into PHP 5.6. As this is a feature addition, the correct branch to target would be master. (It may land in a lower branch with RM approval, but the starting point should be master.)

cjbj · 2017-03-06T21:33:46Z

Tagging @tianfyang

tianfyang · 2017-03-27T07:15:49Z

The implementation looks good!

I just have a question on php_oci_disable_taf_callback(). It seems to unset the user's callback function but callback_fn() in OCI8 driver will still be called. It might be better to set failover.fo_ctx and failover.callback_function to NULL to completely unregister TAF.

Also, as nikic mentioned, the new feature will be merged to master. With PHP7 , MAKE_STD_ZVAL() needs to be removed and ZVAL_RESOURCE needs to replaced by ZVAL_RES.

Can you please add a test to demonstrate the usage of TAF in PHP? Meantime, I will do more testing to ensure the behavior is expected.

nikic · 2017-03-27T17:24:16Z

ext/oci8/oci8_interface.c

+	char *callback = NULL;
+	int callback_len = 0;
+
+	if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "r|s!", &z_connection, &callback, &callback_len) == FAILURE) {


Currently this restricts the callback to being a string. It would be preferable to accept a general "callable" here (such as a closure). For this the f modifier is used together with zend_fcall_info * and zend_fcall_info_cache * arguments. A look at

php-src/ext/standard/array.c

Line 5997 in edfa87e

PHP_FUNCTION(array_map)

might help, though it uses a different parameter parsing API.

Right, I just didn't know how to properly store the zend_fcall_info for later. Also a callback rarely occurs so it probably would be better to store the zend_fcall_info without the cache.

I'm fairly new to PHP extension writing and still don't fully understand the zval reference counter; as the zend_fcall_info contains multiple zval, would I have to increase all of them? And do I actually have to increase the counter after receiving the zend_fcall_info? It's hard to find answers to such questions - or maybe I'm just looking at the wrong locations ^^

I don't have a great answer here -- fcall_info is a bit of a mess. The simplest way to handle this is probably to not bother: Instead accept a zval, use zend_is_callable to make sure it's callable and store that zval (with ZVAL_COPY). Then pass it to call_user_function the way you do now. As you say, this callback will not be commonly invoked, so there is no performance advantage to properly managing fci/fcc here.

KoenigsKind · 2017-03-28T14:17:50Z

@tianfyang

The implementation looks good!

Thanks :)

I just have a question on php_oci_disable_taf_callback(). It seems to unset the user's callback function but callback_fn() in OCI8 driver will still be called. It might be better to set failover.fo_ctx and failover.callback_function to NULL to completely unregister TAF.

php_oci_disable_taf_callback is mainly used to unregister the userspace callback function on a permanent connection when a request ends. By keeping the callback on the driver registered, the workload on the next request is reduced; as it only needs to update the callback at php_oci_connection, but doesn’t need to register the callback on the driver.

Also, as nikic mentioned, the new feature will be merged to master. With PHP7 , MAKE_STD_ZVAL() needs to be removed and ZVAL_RESOURCE needs to replaced by ZVAL_RES.

I will try to implement it for PHP7 :)

Can you please add a test to demonstrate the usage of TAF in PHP?

Do you mean a phpt test? I’ll try to create one, though I don’t know how to cause a failover via PHP.

cjbj · 2017-03-29T06:39:46Z

I don't think committing a phpt test makes a lot of sense since the tests have to run cleanly in lots of different environments. We can add some tests to our not-at-all-portable release suite. However knowing what you've tested, and how, will be helpful.

Added a short and a more detailed example of how to use the TAF Callback. I wasn't able to check if the scripts run through, but the basic concept should be understandable.

KoenigsKind · 2017-03-30T19:31:49Z

@tianfyang @cjbj

However knowing what you've tested, and how, will be helpful.

The basic concept:
I had a pool with two database servers, the script would connect to it and do a slow select statement on it (by sleeping between each fetch). While the script was running, I shut down the server it was connected to and thus forcing a failover.

The PHP files I used got lost through some unfortunate events, but I wrote two example files which should show the way I did it. I wasn't able to test the scripts, so there might still be some typos in them.

Things I've tested

Causing a failover with and without registering callback
Using permanent connections, check if callback function is being unregistered:
- In the same file (same request): register callback, close connection, reopen connection, cause failover
- In multiple files (separate requests): one file registers callback, the other doesn't

tianfyang

Just went back to TAF today from a few other escalated works. Thanks a lot for the examples. I can successfully enable TAF with your implementation now.

I saw a memory leak. Please see my comment inline.

I will do more testing, including threading mode.

tianfyang · 2017-04-05T07:11:33Z

ext/oci8/oci8.c

 #endif /* HAVE_OCI8_DTRACE */
+
+	if (connection->taf_callback) {
+		pefree(connection->taf_callback, connection->is_persistent);


connection->taf_callback needs to be set to NULL, otherwise a memory leak may occur. While disconnecting from a normal connection, php_oci_connection_close() gets called before php_oci_disable_taf_callback(). If connection->taf_callback is not set to NULL, it gets reallocated and filled with PHP_OCI_TAF_DISABLE_CALLBACK in the last call of php_oci_disable_taf_callback(). However, php_oci_connection_close() has already been called at this point and connection->taf_callback will not be freed. Thus, a memory leak occurs.

KoenigsKind · 2017-04-07T17:08:34Z

@nikic
I implemented it now for PHP 7; what would be the best way to update this pull request, as my branch oci8-taf is still on PHP 5?

Would this work?

git checkout oci8-taf
git reset --hard master
git push --force

And then do a commit with my new implementation for PHP 7.
Or would it be better, to create a new pull request using a new branch?

nikic · 2017-04-07T17:15:07Z

@KoenigsKind In this case a new probably PR makes more sense, if only to preserve the PHP 5.6 implementation for future reference. (But force push + change of base branch on github is also fine)

KoenigsKind · 2017-04-07T18:51:54Z

Created new pull request for PHP 7: #2459

cjbj · 2017-04-07T22:03:51Z

@KoenigsKind FYI @tianfyang will be busy/away for a bit, so have patience.

nikic · 2017-05-01T12:23:03Z

Closing this in favor of PR #2459 for PHP 7.

nikic added the Feature label Mar 6, 2017

nikic reviewed Mar 27, 2017

View reviewed changes

Added examples

4327ae4

Added a short and a more detailed example of how to use the TAF Callback. I wasn't able to check if the scripts run through, but the basic concept should be understandable.

tianfyang reviewed Apr 5, 2017

View reviewed changes

KoenigsKind mentioned this pull request Apr 7, 2017

oci8 - Implementation of Oracle TAF Callback (PHP7) #2459

Closed

3 tasks

KoenigsKind changed the title ~~oci8 - Implementation of Oracle TAF Callback~~ oci8 - Implementation of Oracle TAF Callback (PHP5) Apr 8, 2017

nikic closed this May 1, 2017

Conversation

KoenigsKind commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Mar 6, 2017

Uh oh!

cjbj commented Mar 6, 2017

Uh oh!

tianfyang commented Mar 27, 2017

Uh oh!

nikic Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

KoenigsKind Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

nikic Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

KoenigsKind commented Mar 28, 2017

Uh oh!

cjbj commented Mar 29, 2017

Uh oh!

KoenigsKind commented Mar 30, 2017

Uh oh!

tianfyang left a comment

Choose a reason for hiding this comment

Uh oh!

tianfyang Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

KoenigsKind commented Apr 7, 2017

Uh oh!

nikic commented Apr 7, 2017

Uh oh!

KoenigsKind commented Apr 7, 2017

Uh oh!

cjbj commented Apr 7, 2017

Uh oh!

nikic commented May 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

KoenigsKind commented Mar 6, 2017 •

edited

Loading