maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Sergei Golubchik <serg@xxxxxxxxxxxx> writes:

Thanks for review!

What I would now like to do is to discuss how to proceeed (where to push this
basically).

On the one hand, the patch got rather more intrusive than I first wanted. The
main issue is that now the mysys/mystrings/dbug code is linked dynamically by
default, so these additional libmysys.so etc. libraries must be installed by
packagers etc. for things to work; similarly extra -lmysys etc. is needed when
using libmysqld.a.

On the other hand, I think this is generally an improvement, linking code
dynamically rather than statically. Saves duplicating code and so on, and
dynamic linking is where the world is moving (has moved, really).

I even tested the overhead of dynamic linking, it is two clock cycles per
function call into a different library (I plan to write a short blog about
that).

So I think it is mainly a question on where we want to risk pushing this: 5.1,
5.2, or 5.3? We could even add it to the list of patches in the .debs for 5.1.
(BTW, Gentoo people seems to have picked up the patch already in their MySQL
and MariaDB 5.1 packages).

And btw, I would like your help/hints on how to extend mysql_config to provide
the right information for how to link.

With those general remarks, here follow comments, inline:

> Can I branch this your tree from somewhere ?

bzr+ssh://bazaar.launchpad.net/~knielsen/maria/mariadb-5.1-mwl74-libmysqld.so/

>> The plugin may optionally specify @plugin_static_if_no_embedded@ in
>> CFLAGS/CXXFLAGS for the static target; this will avoid needlessly
>> compiling with -fPIC if no libmysqld is being built.
>
> I failed to understand this paragraph :(

With this patch, every source file for a plugin is compiled twice, once with
-fPIC and once without.

However, if we are not building libmysqld, then the -fPIC versions will never
be used.

In this case, @plugin_static_if_no_embedded@ will add -static to CFLAGS,
avoiding the needless compilations with -fPIC. Otherwise it will be empty.

Hope this was clearer.

[There is BTW another similar issue: when building a convenience library,
libtool will again compile sources both with and without -fPIC. But it only
ever uses the -fPIC objects in the convenience library, the objects without
-fPIC are not used. I even tested the newest libtool version, and also found
an email thread discussing this (and agreeing that the problem is there)].

>> This patch only does what is necessary to build libmysqld.so. There
>> are some more cleanups that are possible now that we are using libtool
>> more fully, which will be done in subsequent patches:
>> 
>>  - In libmysql_r/, we should be able to just link libmysys.la etc,
>>  instead of symlinking and re-compiling sources into the directory.
>> 
>>  - In libmysql/, we can similarly avoid symlinking and recompiling
>>  sources if we instead build a libmysys_nothread.la library with
>>  appropriate CFLAGS and link that.
>
> I'd rather not build libmysql at all.
> Or, at least, not by default.

Right, let's add this on top of this patch once we find out where we want to
push it?

If we link libmysql_r with libmysys, it will also solve another issue I have (with
conflicts between C and assembler versions of bmovXXX); I postponed this issue
until we decided about linking libmysql_r with mysys etc (it seems you agree
we should do this).

>> === modified file 'config/ac-macros/plugins.m4'
>> --- config/ac-macros/plugins.m4	2010-09-18 07:53:48 +0000
>> +++ config/ac-macros/plugins.m4	2010-09-27 12:41:05 +0000
>> @@ -115,18 +115,32 @@ dnl ------------------------------------
>>  dnl Macro: MYSQL_PLUGIN_STATIC
>>  dnl
>>  dnl SYNOPSIS
>> -dnl   MYSQL_PLUGIN_STATIC([name],[libmyplugin.a])
>> +dnl   MYSQL_PLUGIN_STATIC([name],[libmyplugin.a],[libmyplugin_embedded.a])
>>  dnl
>>  dnl DESCRIPTION
>> -dnl   Declare the name for the static library 
>> +dnl   Declare the name for the static library
>> +dnl
>> +dnl   Third argument is optional, only needed for special plugins that depend
>> +dnl   on server internals and have source files that must be compiled specially
>> +dnl   with -DEMBEDDED_LIBRARY for embedded server. If specified, the third
>> +dnl   argument is used to link embedded server instead of the second.
>
> Does that mean that all plugin files will be rebuilt with
> -DEMBEDDED_LIBRARY if libmyplugin_embedded.a is specified ?

No. It means that the plugin Makefile.am has the possibility to specify a
different library to be used for embedded.

Later in the patch I modify all plugins that need it (ie. they before
specified MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS) so that they build a
convenience library (once) with all the source files that do not depend on
internals. And compile the few source files that do depend on internals in two
versions with or without -DEMBEDDED_LIBRARY. And then link those together to
two different libraries, one for server, one for embedded.

With this, it's basically up to the plugin Makefile.am to do what it wants.

>> -AC_DEFUN([MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS],[
>> - MYSQL_REQUIRE_PLUGIN([$1])
>> - m4_define([MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS_]AS_TR_CPP([$1]), [$2])
>> -])
>
> I thought we agreed on IRC to keep it.
> Do you mean, you tried it and it didn't work?

Yes.

I tried quite hard to find a way to keep compatibility. But failed :-(

In the end I could not think of anything sensible to do for
MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS(), so I removed it.

This is related to your question below, about keeping compatibility so that
eg. PBXT could still work unmodified between MySQL and MariaDB.

I thought a lot about how to do this, but in the end failed. The hardest
problem is that to link libmysqld.so with libtool, we need plugins to provide
libXXX.la files in MYSQL_PLUGIN_STATIC(). And those libXXX.la simply will not
work with the magic loop in MySQL sources that extracts objects from libXXX.a,
removes duplicates, and links them back again. At least I couldn't see how to
do it.

So in the end I gave up on this compatibility. Sad, however a small comfort is
that

 - This only hits plugins that say MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS(),
   which ideally they shouldn't anyway.

 - This only hits static plugins, which in any case need an incompatible
   maria_declare_plugin() added in the source (though this can be handled with
   #ifdef; I wonder if there is m4/automake equivalent ifdef that could help
   here?)

If you have an idea on how to achieve compatibility and do something sensible
for MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS(), then let me know ...

>> +# __MYSQL_EMIT_CHECK_PLUGIN arguments:
>> +#
>> +#  1 - plugin identifying name
>> +#  2 - plugin identifying name, with `-' replaced by `_'
>> +#  3 - plugin long name
>> +#  4 - plugin description
>> +#  5 - mysql_plugin_define (eg. WITH_xxx_STORAGE_ENGINE)
>> +#  6 - directory
>> +#  7 - static target (if supports static build)
>> +#  8 - dynamic target (if supports dynamic build)
>> +#  9 - mandatory flag
>> +# 10 - disabled flag
>> +# 11 - static target for libmysqld (if different from mysqld)
>
> perhaps "if different from $7" ?

Fixed, thanks.

>> +# 12 - actions
>
> better use 'dnl' here, not '#'
>
> because '#'-lines will go into configure script, where they will mean
> nothing, as the macro will be already expanded.

Fixed

>> +dnl If not building libmysqld embedded server, then there is no need to build
>> +dnl shared object versions of static plugins.
>
> heh :)
> There, on the contrary, I'd rather use '#' for comments, not 'dnl'

Ok, I see, thanks for helping an m4 newbee :-)
Fixed.

>> === modified file 'configure.in'
>> --- configure.in	2010-08-27 14:12:44 +0000
>> +++ configure.in	2010-09-13 20:17:09 +0000
>> @@ -2846,9 +2845,6 @@ if test "$with_server" != "no" -o "$THRE
>>  then
>>    AC_DEFINE([THREAD], [1],
>>              [Define if you want to have threaded code. This may be undef on client code])
>> -  # Avoid _PROGRAMS names
>> -  THREAD_LOBJECTS="thr_alarm.o thr_lock.o thr_mutex.o thr_rwlock.o my_pthread.o my_thr_init.o mf_keycache.o mf_keycaches.o waiting_threads.o"
>> -  AC_SUBST(THREAD_LOBJECTS)
>
> What was that? Definitions for mysys/Makefile.am to know what to build
> (or not) depending on whether threads are enabled ?

Yes. This is fixing an inconsistency in the original sources.

These source files should only be compiled when threads are enabled. There was
code to deal with this in two places: here in configure.in, and in
mysys/Makefile.am. Each place handled a different set of files, and there was
overlap (maybe due to merge error?)

Anyway, with pure static libraries, this overlap did not cause problems
apparently, but with libtool and .so linking, it caused a failure. So I
decided to clean it up to be done in one place only.

>> === modified file 'dbug/Makefile.am'
>> --- dbug/Makefile.am	2010-08-27 14:12:44 +0000
>> +++ dbug/Makefile.am	2010-09-28 11:23:14 +0000
>> @@ -16,10 +16,10 @@
>>  # MA 02111-1307, USA
>>  
>>  INCLUDES =              -I$(top_builddir)/include -I$(top_srcdir)/include
>> -LDADD =                 libdbug.a ../mysys/libmysys.a ../strings/libmystrings.a
>> -pkglib_LIBRARIES =      libdbug.a
>> +LDADD =                 libdbug.la ../mysys/libmysys.la ../strings/libmystrings.la
>
> better to use $(top_builddir)/libmysys.la etc
> (here and below)

Fixed (throughout the patch)

> How's the build time - new, libmysql.so vs. the old libmysqld.a ?

Thanks for remind me, I checked. These are on a quad-core with no ccache:

BUILD/compile-pentium64-valgrind-max:

  Before: 3m19.454s
  After:  4m18.947s

So build time _is_ increased somewhat ...

> Paul would apparently want his engine to work both with MySQL and
> MariaDB. Could we somehow make it possible (applies to other engines
> too)?

(See discussion above)

Thanks,

 - Kristian.