List:Commits« Previous MessageNext Message »
From:Joerg Bruehe Date:February 15 2008 10:40am
Subject:Re: bk commit into 5.0 tree (tsmith:1.2578) BUG#28555
View as plain text  
Hi Tim, all!


I checked your patch, comments are inserted below.

I tested the RPM commands which you added on a SuSE 9.3 installation, 
and they worked fine.

I did not try to verify your complete patch, because I lack a test 
machine (physical or virtual) on which to install a RPM that might 
trigger the check.


tim@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.0 repository of tsmith.  When tsmith does a push these changes
> will be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2008-02-15 02:30:12-07:00, tsmith@stripped +1 -0
>   Bug #28555  Upgrading MySQL Fails to shut down old server and kills socket file
>   
>   Check for an existing MySQL server package from a different vendor or
>   major MySQL version.  In such a case, refuse to install the server and
>   recommend how to safely remove the old packages before installing the
>   new ones.
> 
>   support-files/mysql.spec.sh@stripped, 2008-02-15 02:30:09-07:00,
> tsmith@stripped +61 -1
>     Add to the %pre server scriptlet checks to ensure that we're not
>     upgrading from another vendor's package, or that this is not a
>     major version upgrade.  If an automatic upgrade isn't safe, print
>     basic instructions on how to do a manual upgrade, and bail out.
> 
> diff -Nrup a/support-files/mysql.spec.sh b/support-files/mysql.spec.sh
> --- a/support-files/mysql.spec.sh	2007-11-16 09:36:11 -07:00
> +++ b/support-files/mysql.spec.sh	2008-02-15 02:30:09 -07:00
> @@ -15,6 +15,7 @@
>  # MA  02110-1301  USA.
>  
>  %define mysql_version   @VERSION@
> +%define mysql_vendor    MySQL AB

While that is correct, it still leaves the risk of somebody outside 
MySQL just using the spec file without changing this setting.
If we really want to be safe, we might set it to "unknown" here and then 
override it when we start the RPM build.
This would require additional tool changes (build scripts).

>  
>  # use "rpmbuild --with static" or "rpm --define '_with_static 1'" (for RPM 3.x)
>  # to enable static linking (off by default)
> @@ -69,7 +70,7 @@ License:	%{license}
> 
> Source:		http://www.mysql.com/Downloads/MySQL-@MYSQL_BASE_VERSION@/mysql-%{mysql_version}.tar.gz
>  URL:		http://www.mysql.com/
>  Packager:	MySQL Production Engineering Team <build@stripped>
> -Vendor:		MySQL AB
> +Vendor:		%{mysql_vendor}
>  Provides:	msqlormysql MySQL-server mysql
>  BuildRequires: ncurses-devel
>  Obsoletes:	mysql

ok.

> @@ -420,6 +421,65 @@ touch $RBR%{_sysconfdir}/my.cnf
>  touch $RBR%{_sysconfdir}/mysqlmanager.passwd
>  
>  %pre server
> +# Check if we can safely upgrade.  An upgrade is only safe if it's from one
> +# of our RPMs in the same version family.
> +
> +installed=`rpm -q --whatprovides mysql-server 2> /dev/null`
> +if [ $? -eq 0 -a -n "$installed" ]; then
> +  myversion='%{mysql_version}'
> +  myvendor='%{mysql_vendor}'
> +  vendor=`rpm -q --queryformat='%{VENDOR}' "$installed" 2>&1`
> +  version=`rpm -q --queryformat='%{VERSION}' "$installed" 2>&1`

The above commands work fine on SuSE 9.3 with MySQL 5.1.23-rc installed.

> +
> +  family=`echo $version | sed -e 's,\([0-9][0-9]*\.[0-9][0-9]*\)\..*,\1,'`
> +  new_family=`echo $myversion | sed -e 's,\([0-9][0-9]*\.[0-9][0-9]*\)\..*,\1,'`

You allow for further growth, supporting multi-digit major and minor 
versions. I'm not sure that is needed, but it should not harm.
Still, I propose some changes:
- Anchor the pattern at the beginning of the string.
- Explicitly extend it to the end (yes, matching is greedy, this is more
   for documentation).
- Suppress output in case of non-match,
   later check for non-empty (this may be considered paranoid).
- Restrict the very first digit to the 1..9 range.
- For documentation, I would rename "family" to "old_family", showing
   the difference to "new_family".
   (this causes changes further down - not shown, for clarity).

So my proposal is:
   +
   +  old_family=`echo $version | sed -n -e 
's,^\([1-9][0-9]*\.[0-9][0-9]*\)\..*$,\1,p'`
   +  new_family=`echo $myversion | sed -n -e 
's,^\([1-9][0-9]*\.[0-9][0-9]*\)\..*$,\1,p'`

Yes, I know these checks on "new_family" may be considered overkill, 
because that is derived from a constant string and not from program 
output. Maybe I prefer symmetry too much ...

> +
> +  vendor_error=
> +  if [ "$vendor" != "$myvendor" ]; then
> +    vendor_error="
> +The current MySQL server package is provided by a different
> +vendor ($vendor) than $myvendor.
> +"
> +  fi
> +
> +  family_error=
> +  if [ "$family" != "$new_family" ]; then

Consequence of the above proposal: Replace the last line by
   +  if [ "$old_family" != "$new_family"
   +       -o -z "$old_family" -o -z "$new_family" ]; then

> +    family_error="
> +Upgrading directly from MySQL $family to MySQL $new_family may not
> +be safe in all cases.
> +"
> +  fi
> +
> +  if [ -n "$vendor_error$family_error" ]; then

This may be a style issue ...
I would not combine these two strings into one, rather check them 
separate, but I am sure the above works.

But *if* you prefer concatenating them (both here and in the message 
below), then I would rather do that when building them:

       error_string=""

       if [ vendor-difference ]; then
         error_string="$error_string
     FOO
         "
       fi

       if [ family-difference ]; then
         error_string="$error_string
     BAR
         "

       if [ -n "$error_string" ]; then

One of my reasons is that this approach will make it easier to extend 
the checks, should we add more later.

> +    cat <<HERE >&2
> +
> +******************************************************************
> +A MySQL server package ($installed) is installed.
> +$vendor_error$family_error
> +A manual upgrade is required.  A recommended procedure is:
                                  ^^^
Isn't it "The" recommended procedure ?  Do we have several ?

> +
> +- Ensure that you have a complete, working backup of your data and my.cnf
> +  files
> +- Shut down the MySQL server cleanly
> +- Remove the existing MySQL packages.  Usually this command will
> +  list the packages you should remove:
> +  rpm -qa | grep -i '^mysql-'
> +
> +  You may choose to use 'rpm --nodeps -ev <package-name>' to remove
> +  the package which contains the mysqlclient shared library.  The
> +  library will be reinstalled by the MySQL-shared-compat package.
> +- Install the new MySQL packages supplied by $myvendor
> +- Ensure that the MySQL server is started
> +- Run the 'mysql_upgrade' program

I am torn between two positions:
- Keep it small.
- Provide as many hints as possible.

For "small", your text is fine.

For "many hints", we would need to give separate explanations for 
"different vendor" (directory structure may differ, ...) and
"different family" (run 'mysql_upgrade').
Also, a different vendor might provide RPMs named 'mysql-*' for which we 
have no equivalent (say, a tutorial, a GUI, ...), and we would not in 
general propose to uninstall these.

A compromise might be to add such hints to the "*_error" strings when 
setting them.

As I said: Torn between two positions ...

> +
> +This is a brief description of the upgrade process.  Important details
> +can be found in the MySQL manual, in the Upgrading section.
> +******************************************************************
> +HERE
> +    exit 1
> +  fi
> +fi
> +
>  # Shut down a previously installed server first
>  if test -x %{_sysconfdir}/init.d/mysql
>  then
> 


-- 
Joerg Bruehe, Senior Production Engineer
MySQL AB, www.mysql.com
Office:  (+49 30) 417 01 487     VoIP: 4464@stripped
Thread
bk commit into 5.0 tree (tsmith:1.2578) BUG#28555tim15 Feb
  • Re: bk commit into 5.0 tree (tsmith:1.2578) BUG#28555Joerg Bruehe15 Feb
    • Re: bk commit into 5.0 tree (tsmith:1.2578) BUG#28555Timothy Smith15 Feb
  • Re: bk commit into 5.0 tree (tsmith:1.2578) BUG#28555Sergei Golubchik15 Feb
    • Re: bk commit into 5.0 tree (tsmith:1.2578) BUG#28555Timothy Smith16 Feb