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
