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.

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

Fixed bug #74631 (PDO_PCO with PHP-FPM: OCI environment initialized before PHP-FPM sets it up) #2534

Closed
wants to merge 1 commit into
base: master
from

Conversation

6 participants
@KiNgMaR
Contributor

KiNgMaR commented May 22, 2017

@cjbj

This comment has been minimized.

Contributor

cjbj commented May 23, 2017

/cc @tianfyang: can you review? Maybe a delayed setup similar to PHP OCI8's is more efficient?

@tianfyang

This comment has been minimized.

tianfyang commented Jul 5, 2017

Hi @KiNgMaR the problem is when running PHP-FPM, the process doesn't load environment, right?
I found a workaround to set env variables before PHP-FPM starts up. I tried it in my Linux box and it works for me:
In /etc/init.d/php-fpm, add (beware the space between the dot and the slash)
. /etc/profile
and modify the /etc/profile to add
. /home/user/env.sh
Then modify php configuration file by adding env[MY_ENV_VAR_1] = 'value1' under the [www] section. Lastly, restart the php-fpm.

Can you please try if this workaround works for you?

To me, the issue seems to be a PHP-FPM bug rather than a PDO_OCI bug. Your changes can fix this issue, but it will impact the performance as PDO_OCI would have to create OCI Env handle for each process instead of having only one OCI Env handle in the lifetime of the module.

@remicollet

This comment has been minimized.

Contributor

remicollet commented Jul 5, 2017

IMHO, this is not a bug.

Indeed, environment need to be set before FPM is launched.
BTW, no workaround will solve the need for library path (LD_LIBRARY_PATH to oracle client library)

Linux Distro, for years, use the /etc/sysconfig/php-fpm (loaded from SysV init script or EnvironmentFile systemd unit file) for such need

@KiNgMaR

This comment has been minimized.

Contributor

KiNgMaR commented Jul 5, 2017

It definitely is a bug. It is only noticeable for the NLS_LANG environment variable, because this is read from within OCIEnvCreate (and never again later). The only "workaround" is to disable clear_env in the php-fpm config. However, this means that all variables from a shell may be forwarded into worker processes, which can be an issue.

To summarize, necessary prerequisites to run into the bug:

  • use php-fpm
  • clear_env=yes (default value) in php-fpm config
  • env[NLS_LANG] = ... in php-fpm config

Symptoms:
NLS_LANG setting is not applied, because OCIEnvCreate runs during the MINIT phase, which is always executed before fpm_env_init_child (where the worker process environment is populated).

Possible fix:
Call OCIEnvCreate later. Either on demand or during RINIT. On demand is probably better :)

@KiNgMaR

This comment has been minimized.

Contributor

KiNgMaR commented Jul 5, 2017

Also, in response to @remicollet, LD_LIBRARY_PATH is not needed to use OCI or PDO_OCI. On our systems, /etc/ld.so.conf.d/ is used instead. For our PHP workers, only selected environment variables are set via env[...] = ....

@tianfyang

This comment has been minimized.

tianfyang commented Jul 21, 2017

@KiNgMaR you can still set clear_env = yes in the FPM config file. The workaround I mentioned above is to set environment variables needed for loading modules like PDO_OCI before FPM is started.

With the workaround above, you can set NLS settings in the . /home/user/env.sh, so that PDO_OCI can pick up the settings when is loaded. In php-fpm.config, set clear_env = yes to avoid the worker process accessing the shell environment variables and set your own environment variables for worker processes.

@KiNgMaR

This comment has been minimized.

Contributor

KiNgMaR commented Jul 21, 2017

Hi @tianfyang, that should be a valid workaround. However, in my testing it was not picking up NLS_LANG correctly either. I can try again if needed.
On a second note, as an average user, I would expect NLS_LANG in php-fpm.conf to work not only for oci8, but also for pdo_oci. 😃

@tianfyang

This comment has been minimized.

tianfyang commented Jul 25, 2017

Hi @KiNgMaR , we're still deciding whether we should merge this fix in. I've tested the your code change and it looks good. The only concern is the performance impact. I understand OCI8 is using the same way but for PDO_OCI user, it will be a performance degradation. Will let you know our decision soon.

Just out of curiosity, you seem to be using both OCI8 and PDO_OCI as you've submitted another PR for PDO_OCI (merged). Any particular reason for using both drivers?

@KiNgMaR

This comment has been minimized.

Contributor

KiNgMaR commented Jul 25, 2017

Hi @tianfyang,

  1. I can amend the patch so that the initialization is done on-demand in the realm of pdo_oci_handle_factory instead of MINIT or RINIT. That should eliminate most of the overhead, right?
  2. We have different teams here and most of them started using oci8 when pdo_oci was still marked as unstable. Since then, some simpler applications have switched to pdo_oci.
@tianfyang

This comment has been minimized.

tianfyang commented Jul 27, 2017

Hi @KiNgMaR

We have decided it as a bug and will merge this fix in soon.

Doing the initialization in pdo_oci_handle_factory will be the same to RINIT since an OCI environment has to be created per process.

@php-pulls

This comment has been minimized.

php-pulls commented Aug 22, 2017

Comment on behalf of tianfyan at php.net:

Merged. Thanks for your contribution.

@php-pulls php-pulls closed this Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment