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

KoenigsKind · 2017-04-07T18:49:18Z

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.

Original pull request
Original pull request for PHP 5: #2405

Things to do

config.m4 needs to compile oci8_failover.c
Check if correctly implemented (especially for multithreading)
Add support for registering callback function via array

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 registering callback function via array

KoenigsKind · 2017-04-07T19:19:24Z

Two examples to demonstrate how to use TAF callbacks: examples.zip

The basic concept for testing
I use a cluster 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 is running, I shut down the server it is connected to and thus forcing a failover.

nikic · 2017-04-07T19:34:53Z

ext/oci8/oci8_interface.c

+	zval *z_connection;
+	php_oci_connection *connection;
+	char *callback = NULL;
+	int callback_len = 0;


This should be size_t

nikic · 2017-04-07T19:46:47Z

ext/oci8/oci8_failover.c

+	}
+
+	/* Create zval */
+	zval retval, callback, params[3];


For now we're still on C89, so variable declarations should happen at the top of the function.

Moved variable declarations to the function beginning (C89)

cjbj · 2017-04-28T05:49:44Z

ext/oci8/oci8_failover.c

+ * End:
+ * vim600: noet sw=4 ts=4 fdm=marker
+ * vim<600: noet sw=4 ts=4
+ */


Missing newline at EOF?

tianfyang

Thanks for the examples and updates for PHP7.
Regarding the 3 tasks to do:

config.m4 needs to compile oci8_failover.c
Add oci8_failover.c in config.m4 at Line 338 and 409.
Check if correctly implemented (especially for multithreading)
I've tested the implementation on Apache where fpm processes concurrent requests which registered TAF callbacks individually and it works fine.

At the moment, the implementation looks good. I'm asking an Oracle HA expert to take another look at the code. Meanwhile, I will do stress testing.

Add support for registering callback function via array
Shall we use zval to achieve closure support instead of using zend_fcall_info as Nikita suggested?
I've tried the way Nikita suggested on my side and it seems to work. Please see my changes inline. @nikic Can you please review my changes?
@KoenigsKind If you don't have time to apply the change, I'm also happy to take over the PR and add it from my side.

tianfyang · 2017-05-01T05:33:23Z

ext/oci8/php_oci8_int.h

@@ -531,6 +535,13 @@ ZEND_BEGIN_MODULE_GLOBALS(oci) /* {{{ Module globals */
 	char		*edition;
 ZEND_END_MODULE_GLOBALS(oci) /* }}} */

+/* {{{ transparent failover related prototypes */
+
+int php_oci_register_taf_callback(php_oci_connection *connection, char *callback);


Define callback as zval*

tianfyang · 2017-05-01T05:33:58Z

ext/oci8/php_oci8_int.h

@@ -164,6 +166,8 @@ typedef struct {
 #ifdef HAVE_OCI8_DTRACE
 	char		   *client_id;					/* The oci_set_client_identifier() value */
 #endif
+
+	char		   *taf_callback;				/* The Oracle TAF callback function in the userspace */


Define taf_callback as zval.

tianfyang · 2017-05-01T05:36:50Z

ext/oci8/oci8_interface.c

+{
+	zval *z_connection;
+	php_oci_connection *connection;
+	char *callback = NULL;


Change Line 51-56 to be:

zval *callback; zend_string *callback_name; if (zend_parse_parameters(ZEND_NUM_ARGS(), "r|z!", &z_connection, &callback) == FAILURE) { return; } if (callback) { if(!zend_is_callable(callback, 0, &callback_name)) { php_error_docref(NULL, E_WARNING, "function '%s' is not callable", ZSTR_VAL(callback_name)); zend_string_release(callback_name); RETURN_FALSE; } zend_string_release(callback_name); }

tianfyang · 2017-05-01T05:40:54Z

ext/oci8/oci8_failover.c

+	sb4 returnValue = 0;
+
+	/* Check if userspace callback function was disabled */
+	if (!fo_ctx->taf_callback || !strcmp(PHP_OCI_TAF_DISABLE_CALLBACK, fo_ctx->taf_callback)) {


Change Line 56-69 to be:

if(Z_ISUNDEF(fo_ctx->taf_callback)) { return 0; } /* Initialize zval */ ZVAL_RES(&params[0], fo_ctx->id); ZVAL_LONG(&params[1], fo_event); ZVAL_LONG(&params[2], fo_type); /* Call user function (if possible) */ if (call_user_function(EG(function_table), NULL, &fo_ctx->taf_callback, &retval, 3, params TSRMLS_CC) == FAILURE) { php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unable to call taf callback function, is it defined?"); }

tianfyang · 2017-05-01T05:41:02Z

ext/oci8/oci8_failover.c

+	ZVAL_NULL(&params[0]);
+
+	/* Cleanup */
+	zval_dtor(&callback);


Remove line 80.

tianfyang · 2017-05-01T05:45:40Z

ext/oci8/oci8_failover.c

+int php_oci_register_taf_callback(php_oci_connection *connection, char *callback)
+{
+	sword errstatus;
+	char *oldCallback = NULL;


Change Line 104-129 to be:

int registered = 0; /* temporary failover callback structure */ OCIFocbkStruct failover; if (!callback) { /* Disable callback */ if (Z_ISUNDEF(connection->taf_callback)) { return 0; // Nothing to disable } registered = 1; zval_ptr_dtor(&connection->taf_callback); ZVAL_UNDEF(&connection->taf_callback); } else { if (!Z_ISUNDEF(connection->taf_callback)) { registered = 1; zval_ptr_dtor(&connection->taf_callback); ZVAL_UNDEF(&connection->taf_callback); } /* Set userspace callback function */ ZVAL_COPY(&connection->taf_callback, callback); } /* OCI callback function already registered */ if (registered) { return 0; }

tianfyang · 2017-05-01T05:47:14Z

ext/oci8/oci8_failover.c

+	PHP_OCI_CALL_RETURN(errstatus, OCIAttrSet, (connection->server, (ub4) OCI_HTYPE_SERVER, (void *) &failover, (ub4) 0, (ub4) OCI_ATTR_FOCBK, connection->err));
+
+	if (errstatus  != OCI_SUCCESS) {
+		pefree(connection->taf_callback, connection->is_persistent);


Change Line 141 to be:

zval_ptr_dtor(&connection->taf_callback); ZVAL_UNDEF(&connection->taf_callback);

tianfyang · 2017-05-01T05:52:25Z

ext/oci8/oci8.c

@@ -2225,6 +2246,12 @@ static int php_oci_connection_close(php_oci_connection *connection)
 		connection->client_id = NULL;
 	}
 #endif /* HAVE_OCI8_DTRACE */
+
+	if (connection->taf_callback) {


Change Line 2250-2253 to be:

if (!Z_ISUNDEF(connection->taf_callback)) { zval_ptr_dtor(&connection->taf_callback); ZVAL_UNDEF(&connection->taf_callback); }

Also add the following after Line 1953, 1968 and 1978 to initialize connection->taf_callback:
ZVAL_UNDEF(&connection->taf_callback);

tianfyang · 2017-05-01T05:53:33Z

ext/oci8/oci8.c

@@ -2667,6 +2694,11 @@ static int php_oci_persistent_helper(zval *zv)
 	if (le->type == le_pconnection) {
 		connection = (php_oci_connection *)le->ptr;

+		/* Remove TAF callback function as it's bound to current request */
+		if (connection->used_this_request && connection->taf_callback) {


Change Line 2698 to be:

if (connection->used_this_request && !Z_ISUNDEF(connection->taf_callback)) {

nikic · 2017-05-01T11:19:13Z

@tianfyang Your suggested changes look fine to me. Only comment I have is that in PHP 7 the TSRMLS_CCs aren't necessary.

@tianfyang

Implementation as suggested by @tianfyang oci_register_taf_callback now accepts any type of zval and config.m4 also compiles oci8_failover.c

KoenigsKind · 2017-05-10T12:33:51Z

Sorry, for the delay. Somehow I didn't get a notification...

@tianfyang Thanks for the implementation I applied it and pushed it to the branch. I also added you as a collaborator, so feel free to push changes yourself. If you want I can also transfer this repo to you, so you get the ownership; I guess you would also get the ownership of this PR by doing this.
I only want this feature to be implemented, it doesn't matter who is doing it :)

About the change to zval, I realized it currently uses only one state (undef), this causes the following problem: Let's say we have a script that creates a permanent connection and registers a failover callback function. If we call this script twice:

Using one state (is_undef): After the first request the zval is set to undef, so at the next request the extension doesn't remember, that the c-callback function was already registered.
Using two states (eg is_undef + is_null): After the first request the zval is set to null, so at the next request the extension knows, it can skip registering the c-callback function and simply change the value of the zval.

tianfyang

@KoenigsKind Thanks for applying the change and adding me as a collaborator.

It's really a good point to have a second state to avoid duplicate C callback registration in a persistent connection (which is functionally equivalent to your PHP_OCI_TAF_DISABLE_CALLBACK. Sorry I missed it). Please see my modification inline and feel free to comment or edit if you see anything missing or not making sense.

tianfyang · 2017-05-11T05:39:20Z

ext/oci8/oci8_failover.c


 	/* Default return value */
 	sb4 returnValue = 0;

 	/* Check if userspace callback function was disabled */
-	if (!fo_ctx->taf_callback || !strcmp(PHP_OCI_TAF_DISABLE_CALLBACK, fo_ctx->taf_callback)) {
+	if (Z_ISUNDEF(fo_ctx->taf_callback)) {


Change Line 56 to:

if (Z_ISUNDEF(fo_ctx->taf_callback) || Z_ISNULL(fo_ctx->taf_callback)) {

tianfyang · 2017-05-11T05:41:49Z

ext/oci8/oci8_failover.c


 	/* temporary failover callback structure */
 	OCIFocbkStruct failover;

 	if (!callback) {
 		/* Disable callback */
-		if (!connection->taf_callback || !strcmp(PHP_OCI_TAF_DISABLE_CALLBACK, connection->taf_callback)) {
+		if (Z_ISUNDEF(connection->taf_callback)) {


Change Line 109 to:
if (Z_ISUNDEF(connection->taf_callback) || Z_ISNULL(connection->taf_callback)) {

tianfyang · 2017-05-11T05:54:48Z

ext/oci8/oci8_failover.c

-	}
+		registered = 1;
+		zval_ptr_dtor(&connection->taf_callback);
+		ZVAL_UNDEF(&connection->taf_callback);


Change Line 115 to:
ZVAL_NULL(&connection->taf_callback);

tianfyang · 2017-05-11T05:55:18Z

ext/oci8/oci8_failover.c

+		zval_ptr_dtor(&connection->taf_callback);
+		ZVAL_UNDEF(&connection->taf_callback);
+	} else {
+		if (!Z_ISUNDEF(connection->taf_callback)) {


Change Line 117 to:
if (!Z_ISUNDEF(connection->taf_callback) && !Z_ISNULL(connection->taf_callback)) {

If it's set to null, it should still set registered to 1, so instead I would change the block at line 117 to:

if (!Z_ISUNDEF(connection->taf_callback)) { registered = 1; if (!Z_ISNULL(connection->taf_callback)) { zval_ptr_dtor(&connection->taf_callback); ZVAL_NULL(&connection->taf_callback); } }

tianfyang · 2017-05-11T05:55:53Z

ext/oci8/oci8_failover.c

+		if (!Z_ISUNDEF(connection->taf_callback)) {
+			registered = 1;
+			zval_ptr_dtor(&connection->taf_callback);
+			ZVAL_UNDEF(&connection->taf_callback);


Change Line 120 to:
ZVAL_NULL(&connection->taf_callback);

tianfyang · 2017-05-11T06:01:13Z

ext/oci8/oci8.c

-		connection->taf_callback = NULL;
+	if (!Z_ISUNDEF(connection->taf_callback)) {
+		zval_ptr_dtor(&connection->taf_callback);
+		ZVAL_UNDEF(&connection->taf_callback);
 	}


Change Line 2253 - 2256 to be:

if (!Z_ISUNDEF(connection->taf_callback)) { if (!Z_ISNULL(connection->taf_callback)) /* If it's NULL, then its value should be freed already */ zval_ptr_dtor(&connection->taf_callback); ZVAL_UNDEF(&connection->taf_callback); }

KoenigsKind

A few more things that should be changed. (Together with the review of @tianfyang)

Also Line 96 of php_oci8_int.h can be removed. Couldn't create a comment as the line didn't change in this commit.

KoenigsKind · 2017-05-17T12:30:06Z

ext/oci8/oci8_failover.c

@@ -101,30 +99,33 @@ int php_oci_disable_taf_callback(php_oci_connection *connection)
 int php_oci_register_taf_callback(php_oci_connection *connection, char *callback)


Change Line 99 to:
int php_oci_register_taf_callback(php_oci_connection *connection, zval *callback)

KoenigsKind · 2017-05-17T12:34:51Z

ext/oci8/oci8.c

@@ -2695,7 +2698,7 @@ static int php_oci_persistent_helper(zval *zv)
 		connection = (php_oci_connection *)le->ptr;

 		/* Remove TAF callback function as it's bound to current request */
-		if (connection->used_this_request && connection->taf_callback) {
+		if (connection->used_this_request && !Z_ISUNDEF(connection->taf_callback)) {


Change Line 2701 to:

if (connection->used_this_request && !Z_ISUNDEF(connection->taf_callback) && !Z_ISNULL(connection->taf_callback)) {

tianfyang · 2017-05-22T06:32:19Z

@KoenigsKind I have tested your latest change and it works fine. Can you please apply the change and commit? I believe we are almost good to merge.

cjbj · 2017-05-23T06:16:29Z

ext/oci8/oci8_failover.c

+   +----------------------------------------------------------------------+
+*/
+
+/* $Id$ */


This $Id$ line can be removed

tianfyang · 2017-05-24T06:43:36Z

ext/oci8/php_oci8_int.h

+/* {{{ transparent failover related prototypes */
+
+int php_oci_register_taf_callback(php_oci_connection *connection, zval *callback);
+int php_oci_disable_taf_callback(php_oci_connection *connection);


Can we return bool instead of int here to align with other oci8 functions?

I took a look at the other functions, do you mean to still return an int, but to return 1 on success and 0 on failure?

My mistake. I was looking at the wrong prototype. For php_oci functions, it should return an int and return 0 on success and 1 on failure. Sorry I misled you.

Implemented a second state for php_oci_connection.taf_callback to prevent duplicate C callback registration in a persistent connection. Changed taf callback registration method to simply return 1 on success and 0 on failure.

tianfyang · 2017-05-26T05:15:13Z

ext/oci8/oci8_interface.c

@@ -42,6 +42,29 @@
 #define OCI_STMT_CALL 10
 #endif

+/* {{{ proto bool oci_register_taf_callback( resource connection [, string callback] )


Now it should be:
oci_register_taf_callback( resource connection [, mixed callback]

oci8_failover functions now return 0 on success and 1 on failure

KoenigsKind · 2017-06-14T16:10:10Z

@tianfyang @cjbj

Is there anything left I can help with? I was not able to find anymore bugs.

tianfyang · 2017-06-16T02:17:50Z

We prepared a user doc for this new feature in OCI8 and had a talk with an Oracle HA expert reviewing the implementation and the doc. Everything looks good now. @cjbj will probably do the merge sometime next week.
@KoenigsKind I'm not sure if you need to rebase your branch before we can merge the PR given it's been a while and your branch may be outdated to base branch?

php-pulls · 2017-06-20T04:40:23Z

Comment on behalf of sixd at php.net:

Merged to 7.x - thanks!

cjbj · 2017-08-15T04:29:20Z

@KoenigsKind a new OCI8 2.1.7 bundle on PECL has this change.

KoenigsKind added 2 commits April 7, 2017 20:03

Merge remote-tracking branch 'refs/remotes/php/master'

9e28a67

KoenigsKind mentioned this pull request Apr 7, 2017

oci8 - Implementation of Oracle TAF Callback (PHP5) #2405

Closed

3 tasks

nikic reviewed Apr 7, 2017

View reviewed changes

krakjoe added the Feature label Apr 8, 2017

Syntax changes

bc8cb6d

Moved variable declarations to the function beginning (C89)

cjbj reviewed Apr 28, 2017

View reviewed changes

ext/oci8/oci8_failover.c

* End:

* vim600: noet sw=4 ts=4 fdm=marker

* vim<600: noet sw=4 ts=4

*/

Copy link

Contributor

cjbj Apr 28, 2017

Choose a reason for hiding this comment

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

Missing newline at EOF?

tianfyang reviewed May 1, 2017

View reviewed changes

zval instead of char* and modified config.m4

6a06678

Implementation as suggested by @tianfyang oci_register_taf_callback now accepts any type of zval and config.m4 also compiles oci8_failover.c

tianfyang reviewed May 11, 2017

View reviewed changes

KoenigsKind commented May 17, 2017

View reviewed changes

cjbj reviewed May 23, 2017

View reviewed changes

tianfyang reviewed May 24, 2017

View reviewed changes

tianfyang reviewed May 26, 2017

View reviewed changes

Swapped return value

8d240df

oci8_failover functions now return 0 on success and 1 on failure

php-pulls closed this Jun 20, 2017

		@@ -101,30 +99,33 @@ int php_oci_disable_taf_callback(php_oci_connection *connection)
		int php_oci_register_taf_callback(php_oci_connection connection, char callback)

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

KoenigsKind commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianfyang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented May 1, 2017

Uh oh!

KoenigsKind commented May 10, 2017

Uh oh!

tianfyang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KoenigsKind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianfyang commented May 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KoenigsKind commented Jun 14, 2017

KoenigsKind commented Apr 7, 2017 •

edited

Loading

nikic Apr 7, 2017 •

edited

Loading

tianfyang left a comment •

edited

Loading