Greetings dear devs!
A few days ago I made my first nginx+php-fpm setup, and I soon
realized some gaps of the current FPM implementation. First of all,
the lack of documentation, which of course I know I cannot complain
about, but this forced me to dig into the source code which in turn
motivated me to write this patch, which is good.
The patch (attached) is for the current PHP 5.3 svn branch, but if you
are interested in merging it in I will of course port it to 5.4
branch. I don't see particular reasons to apply it to 5.3, I was
working on it because I'm planning to use it in my production
environments. It would be really nice if it could make it in the 5.4
because it contains also some name changes in the ini file variables
(with BC of course).
The biggest and most interesting change is for sure the introduction
of a [defaults] config section. More about this below, first the
complete changelog of my patch:
Detailed changelog of the patch:
- Renamed options "pm.status_path", "ping.path", "ping.response" into
a new logical group "diagnostics", so they are now respectively:
"diagnostics.status_path", "diagnostics.ping_path",
"diagnostics.ping_response". - Reordered in a more logical way the pool ini options, from a
most-likely-to-be-customized first to the least. Usually when you edit
config file you have big concentration on the first few settings, then
you go like "blah blah defaults defaults" and so on, so this kind of
order makes sense. - Aligned all the code listings of pool options with the "official"
order to ease auditing. The "official" order is the one reported in
the struct definition in fpm_conf.h, which of course is the same as
the php-fpm.conf.in config template - Improved error messages. A better message helps new adopters to get
started quickly, at the beginning I was really puzzled in front of
some not very helpful messages. - Introduced a new section [defaults] that allows setting default values
- Dropped restriction of not setting the same value multiple times,
the last one holds - Removed a lot of redundant checks that are logically implied or not
really required, without reducing the robustness of the program. - Improved a lot code readability in some parts, plus added some
useful comments in the parts that were less easy to understand. - Refactored some functions to be shorter from code lines point of
view, while still doing exactly the same function. - Various white space and cosmetic code improvements
- Moved macros STR2STR, BOOL2STR, and PM2STR from fpm_conf.h to
fpm_conf.c, no reason to make them public since that's the only file
using them,
so in case we need to change them in the future, there is less risk
of breaking something which depends on them. - Fixed a memory leak in fpm_conf_expand_pool_name() (previous
dynamically allocated *value was lost)
Now about the new [defaults] section of the config file.
In the current version if you want to make a decent looking config
file you have to do something like that:
[global]
....your global stuff...
[pool1]
user = pool1
listen = ...
include = pool_defaults.conf
[pool2]
user = pool1
listen = ...
include = pool_defaults.conf
and so on, plus of course you need the external file pool_defaults.conf.
With these changes, you can now do something like this:
[global]
....your global stuff...
[defaults]
....my defaults valid for every pool...
pm.max_children = 500
pm.start_servers = 10
[pool1]
user = pool1
listen = ...
[pool2]
user = pool2
listen = ...
[pool3]
user = pool3
listen = ...
pm.start_servers = 20
There is also a small gain in the parsing time, because defaults are
propagated in memory instead of parsed multiple times as with the
include files solution. I have to admit this argument is quite
irrilevant because it's only a startup time overhead, but it's so much
more elegant in my eyes.
Also by dropping the constraint of setting strings only once, you can
override your defaults in the pools, so you can have an access log
format for all of them except one.
Upcoming changes that would follow from me if you accept this patch:
- allow '$pool' variable to be used in the [globals], but this
requires a bit of restructuring because it needs to be lazy-expanded
in post process time instead of parsing time. - possibility to include more than one file per section
Looking forward to have some feedback from you :)
Kind regards
--
Giovanni Giacobbi
Sorry I always forget this mailing list is "special" and strip my attachments...
Here is a link:
http://pastebin.com/7JpXr22c
Greetings dear devs!
A few days ago I made my first nginx+php-fpm setup, and I soon
realized some gaps of the current FPM implementation. First of all,
the lack of documentation, which of course I know I cannot complain
about, but this forced me to dig into the source code which in turn
motivated me to write this patch, which is good.The patch (attached) is for the current PHP 5.3 svn branch, but if you
are interested in merging it in I will of course port it to 5.4
branch. I don't see particular reasons to apply it to 5.3, I was
working on it because I'm planning to use it in my production
environments. It would be really nice if it could make it in the 5.4
because it contains also some name changes in the ini file variables
(with BC of course).The biggest and most interesting change is for sure the introduction
of a [defaults] config section. More about this below, first the
complete changelog of my patch:Detailed changelog of the patch:
- Renamed options "pm.status_path", "ping.path", "ping.response" into
a new logical group "diagnostics", so they are now respectively:
"diagnostics.status_path", "diagnostics.ping_path",
"diagnostics.ping_response".- Reordered in a more logical way the pool ini options, from a
most-likely-to-be-customized first to the least. Usually when you edit
config file you have big concentration on the first few settings, then
you go like "blah blah defaults defaults" and so on, so this kind of
order makes sense.- Aligned all the code listings of pool options with the "official"
order to ease auditing. The "official" order is the one reported in
the struct definition in fpm_conf.h, which of course is the same as
the php-fpm.conf.in config template- Improved error messages. A better message helps new adopters to get
started quickly, at the beginning I was really puzzled in front of
some not very helpful messages.- Introduced a new section [defaults] that allows setting default values
- Dropped restriction of not setting the same value multiple times,
the last one holds- Removed a lot of redundant checks that are logically implied or not
really required, without reducing the robustness of the program.- Improved a lot code readability in some parts, plus added some
useful comments in the parts that were less easy to understand.- Refactored some functions to be shorter from code lines point of
view, while still doing exactly the same function.- Various white space and cosmetic code improvements
- Moved macros STR2STR, BOOL2STR, and PM2STR from fpm_conf.h to
fpm_conf.c, no reason to make them public since that's the only file
using them,
so in case we need to change them in the future, there is less risk
of breaking something which depends on them.- Fixed a memory leak in fpm_conf_expand_pool_name() (previous
dynamically allocated *value was lost)Now about the new [defaults] section of the config file.
In the current version if you want to make a decent looking config
file you have to do something like that:[global]
...your global stuff...[pool1]
user = pool1
listen = ...
include = pool_defaults.conf[pool2]
user = pool1
listen = ...
include = pool_defaults.confand so on, plus of course you need the external file pool_defaults.conf.
With these changes, you can now do something like this:
[global]
...your global stuff...[defaults]
...my defaults valid for every pool...
pm.max_children = 500
pm.start_servers = 10[pool1]
user = pool1
listen = ...[pool2]
user = pool2
listen = ...[pool3]
user = pool3
listen = ...
pm.start_servers = 20There is also a small gain in the parsing time, because defaults are
propagated in memory instead of parsed multiple times as with the
include files solution. I have to admit this argument is quite
irrilevant because it's only a startup time overhead, but it's so much
more elegant in my eyes.Also by dropping the constraint of setting strings only once, you can
override your defaults in the pools, so you can have an access log
format for all of them except one.Upcoming changes that would follow from me if you accept this patch:
- allow '$pool' variable to be used in the [globals], but this
requires a bit of restructuring because it needs to be lazy-expanded
in post process time instead of parsing time.- possibility to include more than one file per section
Looking forward to have some feedback from you :)
Kind regards
--
Giovanni Giacobbi
--
Giovanni Giacobbi
hi,
It is nothing special, you have to use .txt for the attachment.
It is also slightly better to create a feature requrest/report at
bugs.php.net and attach the patch to it, so it won't get lost into the
MLs archives :)
Thanks for your work!
Cheers,
Sorry I always forget this mailing list is "special" and strip my attachments...
Here is a link:
http://pastebin.com/7JpXr22cGreetings dear devs!
A few days ago I made my first nginx+php-fpm setup, and I soon
realized some gaps of the current FPM implementation. First of all,
the lack of documentation, which of course I know I cannot complain
about, but this forced me to dig into the source code which in turn
motivated me to write this patch, which is good.The patch (attached) is for the current PHP 5.3 svn branch, but if you
are interested in merging it in I will of course port it to 5.4
branch. I don't see particular reasons to apply it to 5.3, I was
working on it because I'm planning to use it in my production
environments. It would be really nice if it could make it in the 5.4
because it contains also some name changes in the ini file variables
(with BC of course).The biggest and most interesting change is for sure the introduction
of a [defaults] config section. More about this below, first the
complete changelog of my patch:Detailed changelog of the patch:
- Renamed options "pm.status_path", "ping.path", "ping.response" into
a new logical group "diagnostics", so they are now respectively:
"diagnostics.status_path", "diagnostics.ping_path",
"diagnostics.ping_response".- Reordered in a more logical way the pool ini options, from a
most-likely-to-be-customized first to the least. Usually when you edit
config file you have big concentration on the first few settings, then
you go like "blah blah defaults defaults" and so on, so this kind of
order makes sense.- Aligned all the code listings of pool options with the "official"
order to ease auditing. The "official" order is the one reported in
the struct definition in fpm_conf.h, which of course is the same as
the php-fpm.conf.in config template- Improved error messages. A better message helps new adopters to get
started quickly, at the beginning I was really puzzled in front of
some not very helpful messages.- Introduced a new section [defaults] that allows setting default values
- Dropped restriction of not setting the same value multiple times,
the last one holds- Removed a lot of redundant checks that are logically implied or not
really required, without reducing the robustness of the program.- Improved a lot code readability in some parts, plus added some
useful comments in the parts that were less easy to understand.- Refactored some functions to be shorter from code lines point of
view, while still doing exactly the same function.- Various white space and cosmetic code improvements
- Moved macros STR2STR, BOOL2STR, and PM2STR from fpm_conf.h to
fpm_conf.c, no reason to make them public since that's the only file
using them,
so in case we need to change them in the future, there is less risk
of breaking something which depends on them.- Fixed a memory leak in fpm_conf_expand_pool_name() (previous
dynamically allocated *value was lost)Now about the new [defaults] section of the config file.
In the current version if you want to make a decent looking config
file you have to do something like that:[global]
...your global stuff...[pool1]
user = pool1
listen = ...
include = pool_defaults.conf[pool2]
user = pool1
listen = ...
include = pool_defaults.confand so on, plus of course you need the external file pool_defaults.conf.
With these changes, you can now do something like this:
[global]
...your global stuff...[defaults]
...my defaults valid for every pool...
pm.max_children = 500
pm.start_servers = 10[pool1]
user = pool1
listen = ...[pool2]
user = pool2
listen = ...[pool3]
user = pool3
listen = ...
pm.start_servers = 20There is also a small gain in the parsing time, because defaults are
propagated in memory instead of parsed multiple times as with the
include files solution. I have to admit this argument is quite
irrilevant because it's only a startup time overhead, but it's so much
more elegant in my eyes.Also by dropping the constraint of setting strings only once, you can
override your defaults in the pools, so you can have an access log
format for all of them except one.Upcoming changes that would follow from me if you accept this patch:
- allow '$pool' variable to be used in the [globals], but this
requires a bit of restructuring because it needs to be lazy-expanded
in post process time instead of parsing time.- possibility to include more than one file per section
Looking forward to have some feedback from you :)
Kind regards
--
Giovanni Giacobbi--
Giovanni Giacobbi--
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Detailed changelog of the patch:
- Renamed options "pm.status_path", "ping.path", "ping.response" into
a new logical group "diagnostics", so they are now respectively:
"diagnostics.status_path", "diagnostics.ping_path",
"diagnostics.ping_response".
I'm not quite sure renaming config options would help anybody, in my opinion it would do more harm than good.
I personally would certainly expect my existing config files to continue to work after upgrade to 5_4.
Yes, we're changing '3' to '4' in the version number, but that doesn't mean we can just go ahead and break the config file.
Other than that, looks ok to me (assuming you've tested it and does continue to work the way it worked before all those cosmetic changes).
--
Wbr,
Antony Dovgal
http://pinba.org - realtime profiling for PHP
FPM is still new, and only just getting the experimental flag removed, now
is the only time to make any BC breaking cosmitic changes that will lead to
less confusion in the long run..
That said - I'm not sure about the diagnostics group name..
Maybe allow for both the old and new naming on 5.4 and remove the old in
5.next...?
Kiall
Detailed changelog of the patch:
- Renamed options "pm.status_path", "ping.path", "ping.response" into
a new logical group "diagnostics", so they are now respectively:
"diagnostics.status_path", "diagnostics.ping_path",
"diagnostics.ping_response".I'm not quite sure renaming config options would help anybody, in my
opinion it would do more harm than good.
I personally would certainly expect my existing config files to continue
to work after upgrade to 5_4.
Yes, we're changing '3' to '4' in the version number, but that doesn't
mean we can just go ahead and break the config file.Other than that, looks ok to me (assuming you've tested it and does
continue to work the way it worked before all those cosmetic changes).--
Wbr,
Antony Dovgalhttp://pinba.org - realtime profiling for PHP
+1 the way for change is to add the new, add an E_DEPRECATED style
warning for a point release or two, and then nix it.
FPM is still new, and only just getting the experimental flag removed, now
is the only time to make any BC breaking cosmitic changes that will lead to
less confusion in the long run..That said - I'm not sure about the diagnostics group name..
Maybe allow for both the old and new naming on 5.4 and remove the old in
5.next...?Kiall
Detailed changelog of the patch:
- Renamed options "pm.status_path", "ping.path", "ping.response" into
a new logical group "diagnostics", so they are now respectively:
"diagnostics.status_path", "diagnostics.ping_path",
"diagnostics.ping_response".I'm not quite sure renaming config options would help anybody, in my
opinion it would do more harm than good.
I personally would certainly expect my existing config files to continue
to work after upgrade to 5_4.
Yes, we're changing '3' to '4' in the version number, but that doesn't
mean we can just go ahead and break the config file.Other than that, looks ok to me (assuming you've tested it and does
continue to work the way it worked before all those cosmetic changes).--
Wbr,
Antony Dovgalhttp://pinba.org - realtime profiling for PHP
2011/7/3 Kiall Mac Innes kiall@managedit.ie:
FPM is still new, and only just getting the experimental flag removed, now
is the only time to make any BC breaking cosmitic changes that will lead to
less confusion in the long run..
You are right. If changes are needed, let's make them now.
That said - I'm not sure about the diagnostics group name..
For the record, when I set them, I spent some time thinking about
their name. I've put status_path in pm section because it's the page
which returns stats for the pool process manager. At the time it was
the right place for status_path. For ping.path and ping.response, as
it was not related to pm, I've put them on their own.
But I'm less convinced with the "path" label (for ping and status).
I'm not sure it's clear. Maybe we can change "path" to "uri".
I think ping.path and ping.response are well placed and named (except
for path, see above), I don't see any reason to change them.
But for status, even if it's related to PM, I'm not user it's relevant
for users. Why, don't change it to status.path (or uri, or else) ?
As this, Process Management has its own section (pm.), so as ping
(ping.), status (status.), listen (listen.) and access logs
(access.*).
But I fact, we should homogenize the configuration file this way by changing:
- rlimit_core to rlimit.core
- rlimit_files to rlimit.files
- slowlog to slowlog.log or slowlog.file or slowlog.path
- request_slowlog_timeout to slowlog.request_timeout
- emergency_restart_threshold to emergency_restart.threshold
- emergency_restart_interval to emergency_restart.interval
++ Jerome
Maybe allow for both the old and new naming on 5.4 and remove the old in
5.next...?Kiall
Detailed changelog of the patch:
- Renamed options "pm.status_path", "ping.path", "ping.response" into
a new logical group "diagnostics", so they are now respectively:
"diagnostics.status_path", "diagnostics.ping_path",
"diagnostics.ping_response".I'm not quite sure renaming config options would help anybody, in my
opinion it would do more harm than good.
I personally would certainly expect my existing config files to continue
to work after upgrade to 5_4.
Yes, we're changing '3' to '4' in the version number, but that doesn't
mean we can just go ahead and break the config file.Other than that, looks ok to me (assuming you've tested it and does
continue to work the way it worked before all those cosmetic changes).--
Wbr,
Antony Dovgalhttp://pinba.org - realtime profiling for PHP
Detailed changelog of the patch:
- Renamed options "pm.status_path", "ping.path", "ping.response" into
a new logical group "diagnostics", so they are now respectively:
"diagnostics.status_path", "diagnostics.ping_path",
"diagnostics.ping_response".I'm not quite sure renaming config options would help anybody, in my opinion
it would do more harm than good.
I personally would certainly expect my existing config files to continue to
work after upgrade to 5_4.
Yes, we're changing '3' to '4' in the version number, but that doesn't mean
we can just go ahead and break the config file.
In fact I don't break them, probably I should've specified it more
clearly, but the previous names are still well accepted:
- /* Backward compatibility options, to be removed in the next major release */
- { "ping.path", &fpm_conf_set_string,
WPO(diag_ping_path) }, - { "ping.response", &fpm_conf_set_string,
WPO(diag_ping_response) }, - { "pm.status_path", &fpm_conf_set_string,
WPO(diag_status_path) },
I can also add a deprecated warning at boot time if you wish. Anyway,
the patch is too big, I admit, if you don't want to accept the change
name I can create another patch without it. But the first time I read
the config file I had a strong feeling that pm.status_path and
ping_path were related that's why i set them to:
/somesecretname/fpm-status
/somesecretname/fpm-ping
Aren't you doing something like that too?
About the testing, well it works, but I didn't test it thoroughly.
Actually I already found a couple of bugs. Who is the official
maintainer of the FPM? If he is OK to merge it then I will do a very
careful test and I will also write some test cases for the config file
parsing (not sure if there is a test suite for that already, didn't
check carefully).
Thank you for your positive feedback :)
--
Giovanni Giacobbi
Detailed changelog of the patch:
- Renamed options "pm.status_path", "ping.path", "ping.response" into
a new logical group "diagnostics", so they are now respectively:
"diagnostics.status_path", "diagnostics.ping_path",
"diagnostics.ping_response".
See my feelings on a previous mail.
- Reordered in a more logical way the pool ini options, from a
most-likely-to-be-customized first to the least. Usually when you edit
config file you have big concentration on the first few settings, then
you go like "blah blah defaults defaults" and so on, so this kind of
order makes sense.
It's a good idea. As the only existent and up to date documentation is
in php-fpm.conf, it became very verbose and hard to read, understand
and tweak. Maybe we should provide a clean default php-fpm.conf and a
full documented version: php-fpm.conf.default and php-fpm.conf.doc.
And of course, we should take time to make some real up to date
documentation (http://fr.php.net/manual/en/install.fpm.php)
- Aligned all the code listings of pool options with the "official"
order to ease auditing. The "official" order is the one reported in
the struct definition in fpm_conf.h, which of course is the same as
the php-fpm.conf.in config template
I've always wanted to do it without taking time to do so.
- Improved error messages. A better message helps new adopters to get
started quickly, at the beginning I was really puzzled in front of
some not very helpful messages.
+1 on principle but I didn't review the patch deeply.
- Introduced a new section [defaults] that allows setting default values
see my comments below
- Dropped restriction of not setting the same value multiple times,
the last one holds
see my comments below
- Removed a lot of redundant checks that are logically implied or not
really required, without reducing the robustness of the program.
I'm OK with that when it's about optimization of running code (main
loop), but if it's for optimizing some init code (before the main
loop) , I don't see the benefit as it can prevent bugs when developing
new feature or correcting bugs. Double checks at init is not that bad
IMO
- Improved a lot code readability in some parts, plus added some
useful comments in the parts that were less easy to understand.
+1 on principle but I didn't review the patch deeply.
- Refactored some functions to be shorter from code lines point of
view, while still doing exactly the same function.
+1 on principle but I didn't review the patch deeply.
- Various white space and cosmetic code improvements
+1
- Moved macros STR2STR, BOOL2STR, and PM2STR from fpm_conf.h to
fpm_conf.c, no reason to make them public since that's the only file
using them,
so in case we need to change them in the future, there is less risk
of breaking something which depends on them.
I've just commit a patch which does that.
(http://svn.php.net/viewvc?view=revision&sortby=date&revision=312922)
- Fixed a memory leak in fpm_conf_expand_pool_name() (previous
dynamically allocated *value was lost)
I've commit the fix.
(http://svn.php.net/viewvc?view=revision&sortby=date&revision=312923)
Now about the new [defaults] section of the config file.
In the current version if you want to make a decent looking config
file you have to do something like that:[global]
....your global stuff...[pool1]
user = pool1
listen = ...
include = pool_defaults.conf[pool2]
user = pool1
listen = ...
include = pool_defaults.confand so on, plus of course you need the external file pool_defaults.conf.
With these changes, you can now do something like this:
[global]
....your global stuff...[defaults]
....my defaults valid for every pool...
pm.max_children = 500
pm.start_servers = 10[pool1]
user = pool1
listen = ...[pool2]
user = pool2
listen = ...[pool3]
user = pool3
listen = ...
pm.start_servers = 20There is also a small gain in the parsing time, because defaults are
propagated in memory instead of parsed multiple times as with the
include files solution. I have to admit this argument is quite
irrilevant because it's only a startup time overhead, but it's so much
more elegant in my eyes.
the parsing time is not a valid argument here :) But your eyes are
(even if I prefer having included files, each and every one of us has
its own preferences)
Also by dropping the constraint of setting strings only once, you can
override your defaults in the pools, so you can have an access log
format for all of them except one.
+1
Upcoming changes that would follow from me if you accept this patch:
- allow '$pool' variable to be used in the [globals], but this
requires a bit of restructuring because it needs to be lazy-expanded
in post process time instead of parsing time.
I don't see where expanding $pool in the [globals] will be used. Only
the following parameter can be set in [globals]:
pid, error_log, log_level, emergency_restart_threshold,
process_control_timeout, daemonize and rlimit*
none of them can use the $pool variable.
Or I missed something.
- possibility to include more than one file per section
You can include many file as you want. You can use glob(3) syntax in one include
include = /path/to/php-fpm/pools/*.conf
or several file in separated includes:
include = /path/to/php-fpm/env.conf
include = /path/to/php-fpm/log.conf
Looking forward to have some feedback from you :)
thx you very much for helping us improving PHP and FPM.
++ jerome