You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Kevac Marko <ma...@kevac.org> on 2009/02/11 22:29:59 UTC
Re: [PATCH] mod_dbd with more than one pool
Now for every exported function we have pair of functions that accepts
pool_name and old one, which is just wrapper:
----
DBD_DECLARE_NONSTD(ap_dbd_t*) ap_dbd_open_pool(apr_pool_t *pool, server_rec *s,
const char *pool_name);
DBD_DECLARE_NONSTD(ap_dbd_t*) ap_dbd_open(apr_pool_t *pool, server_rec *s);
APR_DECLARE_OPTIONAL_FN(ap_dbd_t*, ap_dbd_open_pool, (apr_pool_t*,
server_rec*, const char*));
APR_DECLARE_OPTIONAL_FN(ap_dbd_t*, ap_dbd_open, (apr_pool_t*, server_rec*));
----
DBD_DECLARE_NONSTD(ap_dbd_t*) ap_dbd_open(apr_pool_t *pool, server_rec *s)
{
return ap_dbd_open_pool(pool, s, DBD_DEFAULT_POOL_NAME);
}
In httpd.conf you can create named pool inside <DBDPool pool_name> or
without. In second case pool_name is DBD_DEFAULT_POOL_NAME.
Thus old functions and old configuration is preserved.
What so you think?
Patch is ready, but it needs some testing before posting.
Re: [PATCH] mod_dbd with more than one pool
Posted by Kevac Marko <ma...@kevac.org>.
On Thu, Feb 12, 2009 at 12:43 AM, Brian Akins <br...@akins.org> wrote:
>
> I was looking to do the same thing to mod_memcache (which should be imported
> into trunk, IMO...)
knight@juffin:~/micex/git/apache$ tree modules/memcache/
modules/memcache/
|-- SConscript
|-- mod_memcache.c
`-- mod_memcache.h
Same as mod_dbd :-)
--
Marko Kevac
Re: [PATCH] mod_dbd with more than one pool
Posted by Graham Leggett <mi...@sharp.fm>.
Kevac Marko wrote:
> Thus old functions and old configuration is preserved.
>
> What so you think?
That sounds to me like it would be safe to backport such a thing to
v2.2. +1.
Regards,
Graham
--
Re: [PATCH] mod_dbd with more than one pool
Posted by Graham Leggett <mi...@sharp.fm>.
Kevac Marko wrote:
> There are some bugs in non-threaded build. I am fixing them right now.
Another quick request: is it possible to update the apr_dbd end user
documentation to show the additional container directive, and to explain
how the module should work?
Regards,
Graham
--
Re: [PATCH] mod_dbd with more than one pool
Posted by Kevac Marko <ma...@kevac.org>.
There are some bugs in non-threaded build. I am fixing them right now.
--
Marko Kevac
Re: [PATCH] mod_dbd with more than one pool
Posted by "M. Brian Akins" <br...@akins.org>.
On Mar 5, 2009, at 4:52 PM, Kevac Marko wrote:
>
> 80 columns is a little bit ancient requirement in the world of 22"
> LCDs, but ok, i'll fix that too.
Not when you have 4 code branches side by side...
Also, netbooks are pretty popular as well. Most of my coding is done
on a 15" laptop screen.
--Brian
Re: [PATCH] mod_dbd with more than one pool
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Kevac Marko wrote:
> Prepared statements are not executed, just parsed (simplified), so no,
> it is not important for initialization statement to be before prepared
> statements.
Right; what I wondered was whether you needed to execute some
sort of "magic" initialization statement which would then allow
normal processing, including preparing regular statements, to proceed.
From the bug report (thanks for that) I see your example is setting
character sets with MySQL, which I take it doesn't affect preparing
statements, but I'm not a MySQL expert. Perhaps other DBs in certain
configurations might have initialization requirements that do affect
preparing statements? Since I'm not an expert in all the different
DBs and their possible configurations, I just wanted to open the
subject for discussion. It also seems to me that it doesn't hurt to
do the initializations first, just in case.
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: [PATCH] mod_dbd with more than one pool
Posted by Kevac Marko <ma...@kevac.org>.
On Fri, Mar 6, 2009 at 7:58 PM, Chris Darroch <ch...@pearsoncmg.com> wrote:
> One thought I had overnight is that you might, if you like, want
> to tackle the init SQL patch first, which seems unrelated to the
> multiple pools idea and significantly simpler, easier to review,
> and probably easier to backport as well.
Done.
> Can you provide an example of how such initialization SQL queries
> would be used? And, as a question, might it be necessary for such
> queries to run before any other SQL queries are prepared? IIRC
> in the patch they are run after the other queries are prepared, but
> if they were truly required for connection initialization, they'd
> need to be absolutely first, no?
Prepared statements are not executed, just parsed (simplified), so no,
it is not important for initialization statement to be before prepared
statements.
--
Marko Kevac
Re: [PATCH] mod_dbd with more than one pool
Posted by Marko Kevac <ma...@kevac.org>.
Topic up!
Issue in bugzilla: https://issues.apache.org/bugzilla/show_bug.cgi?id=45456
On Tue, Mar 31, 2009 at 2:08 PM, Kevac Marko <ma...@kevac.org> wrote:
> Nick, any news?
>
> On Fri, Mar 13, 2009 at 3:54 PM, Nick Kew <ni...@webthing.com> wrote:
>> Thanks. I'll test-drive today.
>
> --
> Marko Kevac
>
--
A. Because it breaks the logical sequence of discussion
Q. Why is top posting bad?
Re: [PATCH] mod_dbd with more than one pool
Posted by Kevac Marko <ma...@kevac.org>.
Nick, any news?
On Fri, Mar 13, 2009 at 3:54 PM, Nick Kew <ni...@webthing.com> wrote:
> Thanks. I'll test-drive today.
--
Marko Kevac
Re: [PATCH] mod_dbd with more than one pool
Posted by Kevac Marko <ma...@kevac.org>.
On Fri, Mar 13, 2009 at 2:54 PM, Nick Kew <ni...@webthing.com> wrote:
> Thanks. I'll test-drive today.
Succeeded?
--
Marko Kevac
Re: [PATCH] mod_dbd with more than one pool
Posted by Nick Kew <ni...@webthing.com>.
On 13 Mar 2009, at 10:21, Kevac Marko wrote:
> https://issues.apache.org/bugzilla/show_bug.cgi?id=46827
>
> Implemented. Patch added.
>
> Comments?
Thanks. I'll test-drive today.
--
Nick Kew
Re: [PATCH] mod_dbd with more than one pool
Posted by Kevac Marko <ma...@kevac.org>.
On Tue, Mar 10, 2009 at 10:45 PM, Nick Kew <ni...@webthing.com> wrote:
> Can I throw an alternative suggestion into the ring.
> Instead of running dbd_init_sql_init at the end of
> dbd_construct, run a hook there. Your function then
> runs on that hook, but it enables other modules
> to do their own thing too.
>
> I think it's a mistake in the current mod_dbd that
> dbd_prepared_init isn't hooked, and having the hook
> could've helped with some of the bugs we've seen.
> While you're hacking it would seem like a good
> opportunity to rectify all that!
https://issues.apache.org/bugzilla/show_bug.cgi?id=46827
Implemented. Patch added.
Comments?
--
Marko Kevac
Re: [PATCH] mod_dbd with more than one pool
Posted by Nick Kew <ni...@webthing.com>.
Nick Kew wrote:
> Kevac Marko wrote:
>> Ok, here is sql init statement only patch against trunk:
>> https://issues.apache.org/bugzilla/show_bug.cgi?id=46827
>
> Thanks for the patch!
>
> Can I throw an alternative suggestion into the ring.
> [ ... ]
> Thoughts?
A further thought. I had in mind PR#45407 - mysql losing
prepared statements on reconnect - and possible similar
problems, when I suggested a hook there. Then the driver
just needs to return an error status that says "you may
need to run this hook now", and mod_dbd can do so.
I see you're one of the people who contributed to the
discussion on that bug. Did you bear it in mind in your
new stuff?
--
Nick Kew
Re: [PATCH] mod_dbd with more than one pool
Posted by Nick Kew <ni...@webthing.com>.
Kevac Marko wrote:
> Ok, here is sql init statement only patch against trunk:
> https://issues.apache.org/bugzilla/show_bug.cgi?id=46827
Thanks for the patch!
Can I throw an alternative suggestion into the ring.
Instead of running dbd_init_sql_init at the end of
dbd_construct, run a hook there. Your function then
runs on that hook, but it enables other modules
to do their own thing too.
I think it's a mistake in the current mod_dbd that
dbd_prepared_init isn't hooked, and having the hook
could've helped with some of the bugs we've seen.
While you're hacking it would seem like a good
opportunity to rectify all that!
Thoughts?
--
Nick Kew
Re: [PATCH] mod_dbd with more than one pool
Posted by Kevac Marko <ma...@kevac.org>.
Ok, here is sql init statement only patch against trunk:
https://issues.apache.org/bugzilla/show_bug.cgi?id=46827
P.S. Thanks http://jukka.zitting.name/git/ for git mirrors. I am happy :-)
P.P.S. Probably I should wait for sql init statement only patch
inclusion before posting everything else?
--
Marko Kevac
Re: [PATCH] mod_dbd with more than one pool
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Kevac Marko wrote:
> I'm not experienced in commiting patches to open source projects, so I
> am very thankful for your comments.
No problem -- thanks for pushing this patch along!
One thought I had overnight is that you might, if you like, want
to tackle the init SQL patch first, which seems unrelated to the
multiple pools idea and significantly simpler, easier to review,
and probably easier to backport as well.
Can you provide an example of how such initialization SQL queries
would be used? And, as a question, might it be necessary for such
queries to run before any other SQL queries are prepared? IIRC
in the patch they are run after the other queries are prepared, but
if they were truly required for connection initialization, they'd
need to be absolutely first, no?
> 80 columns is a little bit ancient requirement in the world of 22"
> LCDs, but ok, i'll fix that too.
I mentioned this just because it's part of the httpd style
guidelines, and because there's no need to reformat existing code
which already meets the guideline.
This is off-topic, but personally I like such a limit not only for
the key reasons already mentioned by Brian (i.e., having two or more
versions of the same code or some inter-related files next to each
other), but also because I find it helps curtail poor design on my part.
If I've got a deeply nested set of blocks and I'm hitting up against
the right margin, that's a good hint I need to rethink what I'm doing
and likely refactor it into some smaller functions. It also forces
one to avoid names like foo_bar_baz_wadda_wadda_yippity_boom_de_boom() --
or in Java, fooBarBazWaddaWaddaYippityBoomDeBoom() -- and find something
shorter and more expressive. And if I have a function with a large
number of arguments, it makes me review whether they should really
be part of a single data structure or object; that's been a good
reminder to me over the years of Fred Brooks's old adage about thinking
through one's data structures first.
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: [PATCH] mod_dbd with more than one pool
Posted by Kevac Marko <ma...@kevac.org>.
Chris, thank you. I am impressed.
I'm not experienced in commiting patches to open source projects, so I
am very thankful for your comments.
I'll try to fix things up very soon.
80 columns is a little bit ancient requirement in the world of 22"
LCDs, but ok, i'll fix that too.
--
Marko Kevac
Re: [PATCH] mod_dbd with more than one pool
Posted by Nick Kew <ni...@webthing.com>.
Chris Darroch wrote:
> Over in mod_dbd.h, Nick's old copyright statement is removed.
> That's definitely something which, if needed, should be dealt with
> separately -- I'm not sure if it should be there or not, but it's
> a legal issue not a coding one.
OK, guess it's best if I deal with that one.
Fixed in r750620.
--
Nick Kew
Re: [PATCH] mod_dbd with more than one pool
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Kevac Marko wrote:
> It's great idea. But are you sure that it is good idea to change both
> main httpd.conf and VirtualHost for new DB connection to be added?
> These are separate things... It will introduce problems if, for
> example, httpd.conf is root writable and virtualhost-blabla.conf is
> user writable.
I don't think anything now prevents you from defining DB connection
configurations in the vhost which are private to it, which would
suit your use case, I believe, so long as we retain that option.
I can also imagine that an administrator might want to provide
a "slate" of pre-rolled DB configurations from which vhost users would
be encouraged to choose. That would help reduce a problem you
might otherwise experience, if you had a large number of vhosts and
everyone rolled their own DB configurations, where because each
configuration was slightly different (e.g., different expiry times
or what have you) you'd overwhelm the DB server with connections.
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: [PATCH] mod_dbd with more than one pool
Posted by Kevac Marko <ma...@kevac.org>.
On Thu, Mar 5, 2009 at 10:05 PM, Chris Darroch <ch...@pearsoncmg.com> wrote:
> One difference, I think -- again, after only a quick review -- is
> that what I was trying to solve with the notion of a <DBDGroup>
> container and then a DBDGroup directive was to minimize the number
> of distinct configuration groups and make configuration as simple
> as possible. You could put, say, two <DBDGroup> sets of directives
> into the main server configuration. Then in a virtual host, you'd
> just need to say "DBDGroup foo" to specify which one you wanted
> to use there.
It's great idea. But are you sure that it is good idea to change both
main httpd.conf and VirtualHost for new DB connection to be added?
These are separate things... It will introduce problems if, for
example, httpd.conf is root writable and virtualhost-blabla.conf is
user writable.
> Your <DBDPool> changes seem targeted at the issue of supporting
> multiple connections within, say, a single virtual host. I'm not
> 100% sure if that's the idea, so please correct me if I'm wrong.
> Then callers who want a particular connection pass the DBD "pool"
> name to ap_dbd_open_pool(), etc.
Yes. We configure separate connection pools to separate DBses. MySQL,
PostgreSQL, SQLite for example on different hosts and give them names.
And user can chose connection from which connection pool he wants.
> This seems like a good enhancement to me, certainly, and we've
> seen some requests for it on the mailing list. If we're adding
> such configuration containers, though, I personally would want to
> ensure that we had a way to minimize the number of distinct
> connection pools involved.
>
> It looks to me as though the logic in dbd_post_config() which
> aims to do this, i.e., minimize the number of distinct DBD
> configurations (when using the existing set of directives) is removed
> in the patch. I may be missing something, but I think that's likely
> to be necessary in the future as well, to support existing installations
> and not require administrators to rewrite their configurations
> after upgrading.
I have tried to keep everything concerning group minimization from
original mod_dbd. If not - it is bug.
I don't really like the way that minimization was implemented, but i
have tried to minimize changes to mod_dbd source.
> Suppose you had many virtual hosts and each one needed one or
> both of two types of DB connection. You'd want to be able to specify
> the two types of connection in just two <DBDPool> or <DBDGroup>
> containers at the main server level, it seems to me. Then each
> virtual host could refer back to the ones it required, if, for
> instance, it was adding vhost-specific DBD authentication directives.
But what if owner of blahblah.host.org needs third connection pool and
he can't change httpd.conf?
--
Marko Kevac
Re: [PATCH] mod_dbd with more than one pool
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Kevac Marko wrote:
> Here I am again.
>
> The patch works well for us.
>
> Is there something else that I can do for now?
I'm going to try to find some time to take a look -- thanks for the
patch; it's good to see the <DBDGroup> idea being taken further than
my initial notions.
On an initial quick glance, there are a few things we'll need to
do before we could apply this patch as-is (aside from any questions
of functionality or testing).
There are some whitespace and formatting changes in the patch
which make it a little harder than necessary to comprehend what's
going on. (This is something I'm guilty of as well; I know how
tempting it is to fix whitespace while refactoring.)
For example, ap_dbd_prepare()'s DBD_DECLARE_NONSTD definition
in mod_dbd.c goes from two lines to one; the original was more in
the httpd style. See http://httpd.apache.org/dev/styleguide.html
for the gruesome details. The same is true of various other
function definitions where the arguments spill past 80 columns.
There's a switch statement in dbd_param_int() which has been
reformatting away from the httpd style -- case statements should
be aligned with the parent switch statement. As far as I can see
there are no functional changes here, so this should be left as-is.
Same thing in dbd_param(), I think.
Some of the error messages will need a grammar check, e.g.,
"could not found dbd configuration for pool" should probably be
"could not find DBD configuration for pool".
The DEFAULT_POOL_COUNT macro seems to be unused; that could
be removed.
Over in mod_dbd.h, Nick's old copyright statement is removed.
That's definitely something which, if needed, should be dealt with
separately -- I'm not sure if it should be there or not, but it's
a legal issue not a coding one.
The other changes in mod_dbd.h look to be, at a functional level,
just adding some declarations. However, there's a fair bit of
reformatting going on as well, such as arguments being given names in
existing functions (ap_dbd_cacquire(conn_rec *c) versus the old
ap_dbd_cacquire(conn_rec*)). That's a nice change, but we should
split that out as a separate reformatting patch before making any
functional changes, or else leave it alone and just concentrate on the
functional stuff. One option is a "jumbo" reformatting patch
which precedes all the subsequent functional changes.
As for the non-formatting changes, it would be great to see a
patch with just those pared down to the minimal set of revisions
necessary. From a quick review I get the general drift; there's
a hash of DBD "pools" (not memory pools) for each server config
and legacy DBD config directives, those outside a <DBDPool> container,
are attached to the DEFAULT_DBD_POOL_NAME.
This would seem to correspond pretty nicely with the <DBDGroup>
idea I envisioned back in late 2006; see the end of this email:
http://marc.info/?l=apache-httpd-dev&m=116742014418304&w=2
One difference, I think -- again, after only a quick review -- is
that what I was trying to solve with the notion of a <DBDGroup>
container and then a DBDGroup directive was to minimize the number
of distinct configuration groups and make configuration as simple
as possible. You could put, say, two <DBDGroup> sets of directives
into the main server configuration. Then in a virtual host, you'd
just need to say "DBDGroup foo" to specify which one you wanted
to use there.
Your <DBDPool> changes seem targeted at the issue of supporting
multiple connections within, say, a single virtual host. I'm not
100% sure if that's the idea, so please correct me if I'm wrong.
Then callers who want a particular connection pass the DBD "pool"
name to ap_dbd_open_pool(), etc.
This seems like a good enhancement to me, certainly, and we've
seen some requests for it on the mailing list. If we're adding
such configuration containers, though, I personally would want to
ensure that we had a way to minimize the number of distinct
connection pools involved.
It looks to me as though the logic in dbd_post_config() which
aims to do this, i.e., minimize the number of distinct DBD
configurations (when using the existing set of directives) is removed
in the patch. I may be missing something, but I think that's likely
to be necessary in the future as well, to support existing installations
and not require administrators to rewrite their configurations
after upgrading.
Suppose you had many virtual hosts and each one needed one or
both of two types of DB connection. You'd want to be able to specify
the two types of connection in just two <DBDPool> or <DBDGroup>
containers at the main server level, it seems to me. Then each
virtual host could refer back to the ones it required, if, for
instance, it was adding vhost-specific DBD authentication directives.
The existing mod_dbd will add such authentication directives --
which from mod_dbd's point of view are additional queries to prepare
after connection -- without requiring a whole new DBD connection.
If an administrator might want to apply authentication directives
within a vhost using a non-default DBD connection, or multiple
authn directives using multiple DBD pool, I think this requires a
little thought about how to make it all work.
There also seems to be the introduction of an init_queries
hash and the DBDInitSQL directives, with the idea of SQL queries
which should be executed immediately after connection. That's
an interesting idea, but it needs to be split out from the
DBD pools/groups stuff as a separate patch, I think.
Finally, once we narrow things down to a set of patches ready
for inclusion, there's a matter of testing. My experience with
mod_dbd is that the range of options and contexts makes testing
a little tedious. Aside from the usual (does it compile with -Wall
without warnings) it needs to be compiled and tested in both
threads and no-threads contexts. In each context, one needs to
check with DBDPersist on and off. For each context and persistence
setting, one then needs to check using ap_dbd_acquire() (where
the connection is tied to the request_rec) versus using ap_dbd_open()
and ap_dbd_close() explicitly. I'm willing to deal with some of
this if I can reconstruct all my test harnesses, but needless to
say it'll take a while, so please be patient.
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: [PATCH] mod_dbd with more than one pool
Posted by Kevac Marko <ma...@kevac.org>.
On Thu, Feb 12, 2009 at 12:56 AM, Nick Kew <ni...@webthing.com> wrote:
> Sounds OK in principle for trunk. If you want to post a patch
> against trunk, I'll try and find the time to review it.
Here I am again.
The patch works well for us.
Is there something else that I can do for now?
--
Marko Kevac
Re: [PATCH] mod_dbd with more than one pool
Posted by Nick Kew <ni...@webthing.com>.
On Thu, 12 Feb 2009 00:29:59 +0300
Kevac Marko <ma...@kevac.org> wrote:
> In httpd.conf you can create named pool inside <DBDPool pool_name> or
> without. In second case pool_name is DBD_DEFAULT_POOL_NAME.
>
> Thus old functions and old configuration is preserved.
>
> What so you think?
>
> Patch is ready, but it needs some testing before posting.
Sounds OK in principle for trunk. If you want to post a patch
against trunk, I'll try and find the time to review it.
--
Nick Kew
Application Development with Apache - the Apache Modules Book
http://www.apachetutor.org/
Re: [PATCH] mod_dbd with more than one pool
Posted by "M. Brian Akins" <br...@akins.org>.
On Feb 11, 2009, at 6:22 PM, Graham Leggett wrote:
>
> Would it make sense for mod_memcache to become a provider beneath
> mod_socache, or am I missing something?
mod_memcache really just provides the config "glue" for apr_memcache
so that every module that wants to use apr_memcache doesn't have to
write all the config "glue" themselves. Yeah, it could probably add
some socache hooks as well. I'm adding some Lua glue right now.
mod_memcache is already Apache licensed.
Re: [PATCH] mod_dbd with more than one pool
Posted by Graham Leggett <mi...@sharp.fm>.
Brian Akins wrote:
> I was looking to do the same thing to mod_memcache (which should be imported
> into trunk, IMO...)
Would it make sense for mod_memcache to become a provider beneath
mod_socache, or am I missing something?
Regards,
Graham
--
Re: [PATCH] mod_dbd with more than one pool
Posted by Brian Akins <br...@akins.org>.
On 2/11/09 4:29 PM, "Kevac Marko" <ma...@kevac.org> wrote:
> What so you think?
>
> Patch is ready, but it needs some testing before posting.
+1
I was looking to do the same thing to mod_memcache (which should be imported
into trunk, IMO...)