[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Please allow postgresql-common 93 into lenny



Hello release team,

I recently fixed a range of PostgreSQL cluster upgrade bugs, reported
by people who did an Etch -> Lenny upgrade. None of them are really
unrecoverable (due to the way pg_upgradecluster works and keeps the
old cluster around, etc.), but some are quite inconvenient to work
around, this I'd like to see them fixed in lenny.

| postgresql-common (93) unstable; urgency=low
| 
|   * t/060_obsolete_confparams.t: Test a direct upgrade from oldest to newest
|     version in addition to consecutive version-by-version upgrades. This
|     checks that parameters which changed several times between the versions
|     are correctly converted. Reproduces #502106.
|   * pg_upgradecluster: Re-read configuration file after doing the "syslog" ->
|     "redirect_stderr" migration, so that the followup "redirect_stderr" ->
|     "logging_collector" rename will actually be done. This fixes a direct 7.4
|     -> 8.3 upgrade. (Closes: #502106)

Obvious one-liner fix (first hunk in pg_upgradecluster diff), longish
test case diff (but that doesn't matter at runtime). The lenny version
does not re-read the parsed configuration file, so if a parameter got
renamed twice across several major releases, as in this case, the
upgraded cluster got an invalid configuration file.

| 
|  -- Martin Pitt <mpitt@debian.org>  Sun, 16 Nov 2008 19:45:34 +0100
| 
| postgresql-common (92) unstable; urgency=low
| 
|   * debian/supported-versions: Add Ubuntu 8.10 and 9.04.

This code path doesn't get run on Debian, zero regression potential.

|   * pg_upgradecluster: Clarify POD about manual mode of the old cluster.
|     Thanks to Toni Mueller for noticing.

Just documentation.

|   * Demote ssl-cert Depends: to Recommends:. (Closes: #498406)

Harmless, and doesn't actually change anything until the
postgresql-8.3 dependency is dropped as well (but I don't actually
plan to do this for lenny, since ensuring that upgrades don't break
with just Recommends isn't actually trivial).

|   * pg_upgradecluster: Provide --locale and -lc-* options similar to
|     pg_createcluster, to provide easy UTF-8 migration from previous
|     legacy-encoded clusters. (Closes: #505785)
|   * t/052_upgrade_encodings.t: Test cases for pg_upgradecluster default and
|     explicit encoding behaviour.

This is probably the most debatable change for Lenny. The problem is
that current pg_upgradecluster always keeps the original
locale/encoding, so there is no automatic way to move to a modern
UTF-8 system. People who have to do this need to run the upgrade
completely manually. Since it is a new feature, it got an entirely new
and thorough test case which also verifies that the original behaviour
still works correctly.

If this is the only reason for rejection, I'm willing to back this out
and upload the other fixes to t-p-u.

|   * debian/init.d-functions: Use --force for forcefully stopping a running
|     cluster for "restart" init.d operation as well. Otherwise restart is not
|     guaranteed to succeed and could hang on existing connections.
|     (Closes: #481359)

Current version can hang "restart" indefinitively if there are
connected clients, which violates the Debian Policy at least in
spirit.

|   * t/052_upgrade_encodings.t: Test upgrading of an SQL_ASCII database in a
|     cluter running under a proper locale. This reproduces #505449.
|   * pg_upgradecluster: Preserve SQL_ASCII encoded databases on upgrade.
|     (Closes: #505449)

Pretty small fix (hunks 5 and 6 in pg_upgradecluster, which read the
database encoding from PostgreSQL and check if it is SQL_ASCII), long
test case.

This is pretty serious, many people run ASCII-encoded databases for
various reasons, and breaking them isn't nice.

| 
|  -- Martin Pitt <mpitt@debian.org>  Sun, 16 Nov 2008 13:42:31 +0100

I ran the test suite with 7.4, 8.1, and 8.3 installed, and everything
(> 1000 tests) passes.

Thanks for considering,

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
diff -Nru postgresql-common-91/debian/changelog postgresql-common-93/debian/changelog
--- postgresql-common-91/debian/changelog	2008-09-07 14:22:10.000000000 +0200
+++ postgresql-common-93/debian/changelog	2008-11-16 19:45:39.000000000 +0100
@@ -1,3 +1,38 @@
+postgresql-common (93) unstable; urgency=low
+
+  * t/060_obsolete_confparams.t: Test a direct upgrade from oldest to newest
+    version in addition to consecutive version-by-version upgrades. This
+    checks that parameters which changed several times between the versions
+    are correctly converted. Reproduces #502106.
+  * pg_upgradecluster: Re-read configuration file after doing the "syslog" ->
+    "redirect_stderr" migration, so that the followup "redirect_stderr" ->
+    "logging_collector" rename will actually be done. This fixes a direct 7.4
+    -> 8.3 upgrade. (Closes: #502106)
+
+ -- Martin Pitt <mpitt@debian.org>  Sun, 16 Nov 2008 19:45:34 +0100
+
+postgresql-common (92) unstable; urgency=low
+
+  * debian/supported-versions: Add Ubuntu 8.10 and 9.04.
+  * pg_upgradecluster: Clarify POD about manual mode of the old cluster.
+    Thanks to Toni Mueller for noticing.
+  * Demote ssl-cert Depends: to Recommends:. (Closes: #498406)
+  * pg_upgradecluster: Provide --locale and -lc-* options similar to
+    pg_createcluster, to provide easy UTF-8 migration from previous
+    legacy-encoded clusters. (Closes: #505785)
+  * t/052_upgrade_encodings.t: Test cases for pg_upgradecluster default and
+    explicit encoding behaviour.
+  * debian/init.d-functions: Use --force for forcefully stopping a running
+    cluster for "restart" init.d operation as well. Otherwise restart is not
+    guaranteed to succeed and could hang on existing connections.
+    (Closes: #481359)
+  * t/052_upgrade_encodings.t: Test upgrading of an SQL_ASCII database in a
+    cluter running under a proper locale. This reproduces #505449.
+  * pg_upgradecluster: Preserve SQL_ASCII encoded databases on upgrade.
+    (Closes: #505449)
+
+ -- Martin Pitt <mpitt@debian.org>  Sun, 16 Nov 2008 13:42:31 +0100
+
 postgresql-common (91) unstable; urgency=low
 
   * Update Brazilian Portugese debconf translations, thanks Eder L. Marques!
diff -Nru postgresql-common-91/debian/control postgresql-common-93/debian/control
--- postgresql-common-91/debian/control	2008-09-06 10:43:03.000000000 +0200
+++ postgresql-common-93/debian/control	2008-11-15 12:25:45.000000000 +0100
@@ -10,7 +10,8 @@
 Architecture: all
 Conflicts: postgresql (<< 7.5), postgresql-client (<< 7.5), postgresql-7.4 (<< 1:7.4.8-10), postgresql-8.0 (<< 8.0.3-7)
 Replaces: postgresql (<< 7.5), postgresql-client (<< 7.5)
-Depends: postgresql-client-common (>= ${source:Version}), procps, adduser, debconf (>= 0.5.00) | debconf-2.0, lsb-base (>= 3.0-3), ssl-cert (>= 1.0.11)
+Depends: postgresql-client-common (>= ${source:Version}), procps, adduser, debconf (>= 0.5.00) | debconf-2.0, lsb-base (>= 3.0-3)
+Recommends: ssl-cert (>= 1.0.11)
 Description: PostgreSQL database-cluster manager
  The postgresql-common package provides a structure under which
  multiple versions of PostgreSQL may be installed and/or multiple
diff -Nru postgresql-common-91/debian/init.d-functions postgresql-common-93/debian/init.d-functions
--- postgresql-common-91/debian/init.d-functions	2008-03-30 17:50:01.000000000 +0200
+++ postgresql-common-93/debian/init.d-functions	2008-11-16 11:36:49.000000000 +0100
@@ -29,7 +29,7 @@
 
         log_progress_msg "$name"
 	set +e
-	if [ "$1" = "stop" ]; then
+	if [ "$1" = "stop" ] || [ "$1" = "restart" ]; then
 	    ERRMSG=$(pg_ctlcluster --force "$2" "$name" $1 2>&1)
 	else
 	    ERRMSG=$(pg_ctlcluster "$2" "$name" $1 2>&1)
diff -Nru postgresql-common-91/debian/supported-versions postgresql-common-93/debian/supported-versions
--- postgresql-common-91/debian/supported-versions	2008-01-20 11:07:47.000000000 +0100
+++ postgresql-common-93/debian/supported-versions	2008-11-15 12:07:58.000000000 +0100
@@ -15,7 +15,7 @@
         7.04 | 7.10)
             /bin/echo -e "8.2\n8.3"
             ;;
-        8.04)
+        8.04 | 8.10 | 9.04)
             /bin/echo -e "8.3"
             ;;
         *)
diff -Nru postgresql-common-91/pg_upgradecluster postgresql-common-93/pg_upgradecluster
--- postgresql-common-91/pg_upgradecluster	2008-03-31 11:32:30.000000000 +0200
+++ postgresql-common-93/pg_upgradecluster	2008-11-16 19:18:40.000000000 +0100
@@ -83,6 +83,7 @@
         # make sure log file output actually gets enabled
         PgCommon::set_conf_value $newversion, $cluster, 'postgresql.conf',
             'redirect_stderr', 'true';
+        %c = read_cluster_conf_file $newversion, $cluster, 'postgresql.conf';
     }
 
     # sort_mem -> work_mem transition
@@ -204,8 +205,23 @@
 
 $newversion = get_newest_version;
 
-exit 1 unless GetOptions ('v|version=s' => \$newversion);
-($newversion) = $newversion =~ /^(\d+\.\d+)$/; # untaint
+my ($locale, $lc_collate, $lc_ctype, $lc_messages, $lc_monetary, $lc_numeric,
+    $lc_time);
+exit 1 unless GetOptions ('v|version=s' => \$newversion,
+    'locale=s' => \$locale, 'lc-collate=s' => \$lc_collate, 
+    'lc-ctype=s' => \$lc_ctype, 'lc-messages=s' => \$lc_messages,
+    'lc-monetary=s' => \$lc_monetary, 'lc-numeric=s' => \$lc_numeric,
+    'lc-time=s' => \$lc_time);
+
+# untaint
+($newversion) = $newversion =~ /^(\d+\.\d+)$/;
+($locale) = $locale =~ /^([\w@._-]+)$/ if $locale;
+($lc_collate) = $lc_collate =~ /^([\w@._-]+)$/ if $lc_collate;
+($lc_ctype) = $lc_ctype =~ /^([\w@._-]+)$/ if $lc_ctype;
+($lc_messages) = $lc_messages =~ /^([\w@._-]+)$/ if $lc_messages;
+($lc_monetary) = $lc_monetary =~ /^([\w@._-]+)$/ if $lc_monetary;
+($lc_numeric) = $lc_numeric =~ /^([\w@._-]+)$/ if $lc_numeric;
+($lc_time) = $lc_time =~ /^([\w@._-]+)$/ if $lc_time;
 
 if ($#ARGV < 1) {
     print "Usage: $0 [-v <newversion>] <version> <cluster name> [<new data directory>]\n";
@@ -224,9 +240,9 @@
 
 my $encoding = get_db_encoding $version, $cluster, 'template1';
 error 'could not get cluster default encoding' unless $encoding;
-my ($lc_ctype, $lc_collate) = get_cluster_locales $version, $cluster;
-error 'could not get cluster locale' unless $lc_ctype;
-error 'could not get cluster collating locale' unless $lc_collate;
+my ($old_lc_ctype, $old_lc_collate) = get_cluster_locales $version, $cluster;
+error 'could not get cluster locale' unless $old_lc_ctype;
+error 'could not get cluster collating locale' unless $old_lc_collate;
 
 if (cluster_exists $newversion, $cluster) {
     error "target cluster $newversion/$cluster already exists";
@@ -271,15 +287,22 @@
 exit 1 if $?;
 
 # create new cluster, preserving encoding and locales
-
 my @argv = ('pg_createcluster', '-u', $info{'owneruid'}, '-g', $info{'ownergid'},
-    '--socketdir', $info{'socketdir'}, '--encoding', $encoding, $newversion,
+    '--socketdir', $info{'socketdir'}, $newversion,
     $cluster);
-push @argv, '--datadir', $datadir if $datadir;
 
+push @argv, ('--datadir', $datadir) if $datadir;
+push @argv, ('--encoding', $encoding) unless $locale or $lc_ctype;
+$lc_ctype ||= $locale || $old_lc_ctype;
+$lc_collate ||= $locale || $old_lc_collate;
+push @argv, ('--locale', $locale) if $locale;
+push @argv, ('--lc-collate', $lc_collate) if $lc_collate;
+push @argv, ('--lc-ctype', $lc_ctype) if $lc_ctype;
+push @argv, ('--lc-messages', $lc_messages) if $lc_messages;
+push @argv, ('--lc-monetary', $lc_monetary) if $lc_monetary;
+push @argv, ('--lc-numeric', $lc_numeric) if $lc_numeric;
+push @argv, ('--lc-time', $lc_time) if $lc_time;
 delete $ENV{'LC_ALL'};
-$ENV{'LC_CTYPE'} = $lc_ctype;
-$ENV{'LC_COLLATE'} = $lc_collate;
 error "Could not create target cluster" if system @argv;
 
 @argv = ('pg_ctlcluster', $newversion, $cluster, 'start');
@@ -337,13 +360,14 @@
     my %databases;
     open F, '-|', $oldpsql, '-h', $oldsocket, '-p', $info{'port'}, 
         '-F|', '-d', 'template1', '-Atc', 
-        'select datname, datallowconn, usename from pg_database, pg_user where datdba = usesysid' or 
+        'select datname, datallowconn, pg_catalog.pg_encoding_to_char(encoding), usename from pg_database, pg_user where datdba = usesysid' or 
         error 'Could not get pg_database list';
     while (<F>) {
         chomp;
-        my ($n, $a, $o) = split '\|';
+        my ($n, $a, $e, $o) = split '\|';
         ($o) = $o =~ /^(.*)$/; # untaint
-        $databases{$n} = [$a eq 't', $o];
+        ($e) = $e =~ /^([\w_]+)$/; # untaint
+        $databases{$n} = [$a eq 't', $o, $e];
     }
     close F;
     error 'could not get list of databases' if $?;
@@ -410,7 +434,7 @@
             }
         }
 
-        if ($lc_ctype eq 'C') {
+        if ($lc_ctype eq 'C' or ${$databases{$db}}[2] eq 'SQL_ASCII') {
             # keep DB encoding exactly as they were
             push @restore_argv, ('-d', 'template1', '-C');
         } else {
@@ -557,12 +581,43 @@
 port since the upgraded one will use the original port. The old cluster is not
 automatically removed. After upgrading, please verify that the new cluster
 indeed works as expected; if so, you should remove the old cluster with
-L<pg_dropcluster(8)>.
+L<pg_dropcluster(8)>. Please note that the old cluster is set to "manual"
+startup mode, in order to avoid inadvertently changing it; this means that it
+will not be started automatically on system boot, and you have to use
+L<pg_ctlcluster(8)> to start/stop it. See section "STARTUP CONTROL" in
+L<pg_createcluster(8)> for details.
 
 The I<newdatadir> argument can be used to specify a non-default data directory
 of the upgraded cluster. It is passed to B<pg_createcluster>. If not specified,
 this defaults to /var/lib/postgresql/I<version>/I<name>.
 
+=head1 OPTIONS
+
+=over 4
+
+=item B<-v> I<newversion>
+
+Set the version to upgrade to (default: latest available).
+
+=item B<--locale=>I<locale>
+
+Set the default locale for the upgraded database cluster. If this option is not
+specified, the locale is inherited from the old cluster.
+
+=item B<--lc-collate=>I<locale>
+
+=item B<--lc-ctype=>I<locale>
+
+=item B<--lc-messages=>I<locale>
+
+=item B<--lc-monetary=>I<locale>
+
+=item B<--lc-numeric=>I<locale>
+
+=item B<--lc-time=>I<locale>
+
+Like B<--locale>, but only sets the locale in the specified category.
+
 =head1 HOOK SCRIPTS
 
 Some PostgreSQL extensions like PostGIS need metadata in auxiliary tables which
diff -Nru postgresql-common-91/t/052_upgrade_encodings.t postgresql-common-93/t/052_upgrade_encodings.t
--- postgresql-common-91/t/052_upgrade_encodings.t	1970-01-01 01:00:00.000000000 +0100
+++ postgresql-common-93/t/052_upgrade_encodings.t	2008-11-16 13:13:50.000000000 +0100
@@ -0,0 +1,77 @@
+# Test default and explicit encoding on upgrades
+
+use strict; 
+
+use lib 't';
+use TestLib;
+
+use Test::More tests => ($#MAJORS == 0) ? 1 : 45;
+
+use lib '/usr/share/postgresql-common';
+use PgCommon;
+
+if ($#MAJORS == 0) {
+        pass 'only one major version installed, skipping upgrade tests';
+        exit 0;
+}
+
+my $outref;
+my $oldv = $MAJORS[0];
+my $newv = $MAJORS[-1];
+
+is ((exec_as 0, "pg_createcluster --start --locale=ru_RU $oldv main", $outref), 0,
+    "creating ru_RU $oldv cluster");
+
+is ((exec_as 'postgres', 'psql -c "create database latintest" template1', $outref), 0,
+    "creating latintest DB with LATIN encoding");
+is ((exec_as 'postgres', 'psql -c "create database asctest encoding = \'SQL_ASCII\'" template1', $outref), 0,
+    "creating asctest DB with ASCII encoding");
+
+is ((exec_as 'postgres', "printf 'A\\324B' | psql -c \"create table t(x varchar); copy t from stdin\" latintest", $outref), 
+    0, 'write LATIN database content to latintest');
+is ((exec_as 'postgres', "printf 'A\\324B' | psql -c \"create table t(x varchar); copy t from stdin\" asctest", $outref), 
+    0, 'write LATIN database content to asctest');
+
+is_program_out 'postgres', "echo \"select * from t\" | psql -Atq latintest",
+    0, "A\324B\n", 'old latintest DB has correctly encoded string';
+is_program_out 'postgres', "echo \"select * from t\" | psql -Atq asctest",
+    0, "A\324B\n", 'old asctest DB has correctly encoded string';
+
+is ((exec_as 'postgres', 'psql -Atl', $outref), 0, 'psql -Atl on old cluster');
+ok ((index $$outref, 'latintest|postgres|ISO_8859_5') >= 0, 'latintest is LATIN encoded');
+ok ((index $$outref, 'asctest|postgres|SQL_ASCII') >= 0, 'asctest is ASCII encoded');
+ok ((index $$outref, 'template1|postgres|ISO_8859_5') >= 0, 'template1 is LATIN encoded');
+
+# upgrade without specifying locales, should be kept
+like_program_out 0, "pg_upgradecluster -v $newv $oldv main", 0, qr/^Success/im;
+
+is ((exec_as 'postgres', "psql --cluster $newv/main -Atl", $outref), 0, 'psql -Atl on upgraded cluster');
+ok ((index $$outref, 'latintest|postgres|ISO_8859_5') >= 0, 'latintest is LATIN encoded');
+ok ((index $$outref, 'asctest|postgres|SQL_ASCII') >= 0, 'asctest is ASCII encoded');
+ok ((index $$outref, 'template1|postgres|ISO_8859_5') >= 0, 'template1 is LATIN encoded');
+is_program_out 'postgres', "echo \"select * from t\" | psql --cluster $newv/main -Atq latintest",
+    0, "A\324B\n", 'new latintest DB has correctly encoded string';
+
+is ((system "pg_dropcluster --stop $newv main"), 0, 'dropping upgraded cluster');
+is ((system "pg_ctlcluster $oldv main start"), 0, 'restarting old cluster');
+
+# upgrade with explicitly specifying other locale
+like_program_out 0, "pg_upgradecluster --locale ru_RU.UTF-8 -v $newv $oldv main", 0, qr/^Success/im;
+
+is ((exec_as 'postgres', "psql --cluster $newv/main -Atl", $outref), 0, 'psql -Atl on upgraded cluster');
+ok (($$outref =~ 'latintest|postgres|(UTF8|UNICODE)') >= 0, 'latintest is UTF8 encoded');
+ok ((index $$outref, 'asctest|postgres|SQL_ASCII') >= 0, 'asctest is ASCII encoded');
+ok (($$outref =~ 'template1|postgres|(UTF8|UNICODE)') >= 0, 'template1 is UTF8 encoded');
+is_program_out 'postgres', "echo \"select * from t\" | psql --cluster $newv/main -Atq latintest",
+    0, "AдB\n", 'new latintest DB has correctly encoded string';
+# ASCII databases don't do automatic encoding conversion, so this remains LATIN
+is_program_out 'postgres', "echo \"select * from t\" | psql --cluster $newv/main -Atq asctest",
+    0, "A\324B\n", 'new asctest DB has correctly encoded string';
+
+is ((system "pg_dropcluster --stop $newv main"), 0, 'dropping upgraded cluster');
+
+is ((system "pg_dropcluster $oldv main"), 0, 'dropping old cluster');
+
+check_clean;
+
+# vim: filetype=perl
diff -Nru postgresql-common-91/t/060_obsolete_confparams.t postgresql-common-93/t/060_obsolete_confparams.t
--- postgresql-common-91/t/060_obsolete_confparams.t	2008-03-30 18:25:58.000000000 +0200
+++ postgresql-common-93/t/060_obsolete_confparams.t	2008-11-16 19:22:24.000000000 +0100
@@ -7,7 +7,7 @@
 use lib 't';
 use TestLib;
 
-use Test::More tests => ($#MAJORS == 0) ? 1 : (12 + $#MAJORS * 9);
+use Test::More tests => ($#MAJORS == 0) ? 1 : (23 + $#MAJORS * 9);
 
 if ($#MAJORS == 0) {
     pass 'only one major version installed, skipping upgrade tests';
@@ -526,14 +526,10 @@
 custom_variable_classes = 'foo'
 ";
 
-# create cluster for oldest version
-is ((system "pg_createcluster $MAJORS[0] main >/dev/null"), 0, "pg_createcluster $MAJORS[0] main");
-
-# Loop over all but the latest major version
-my @testversions = sort @MAJORS;
-while ($#testversions) {
-    my $cur = shift @testversions;
-    my $new = $testversions[0];
+# Test one particular upgrade (old version, new version)
+sub do_upgrade {
+    my $cur = $_[0];
+    my $new = $_[1];
 
     # Write configuration file and start
     my $datadir = PgCommon::cluster_data_directory $cur, 'main';
@@ -563,9 +559,34 @@
         "Stopping new $new pg_ctlcluster");
 }
 
+# create cluster for oldest version
+is ((system "pg_createcluster $MAJORS[0] main >/dev/null"), 0, "pg_createcluster $MAJORS[0] main");
+
+# Loop over all but the latest major version
+my @testversions = sort @MAJORS;
+while ($#testversions) {
+    my $cur = shift @testversions;
+    my $new = $testversions[0];
+    do_upgrade $cur, $new;
+}
+
 # remove latest cluster and directory
 is ((system "pg_dropcluster $testversions[0] main"), 0, 'Dropping remaining cluster');
 
+# now test a direct upgrade from oldest to newest, to also catch parameters
+# which changed several times, like syslog -> redirect_stderr ->
+# logging_collector
+if ($#MAJORS > 1) {
+    is ((system "pg_createcluster $MAJORS[0] main >/dev/null"), 0, "pg_createcluster $MAJORS[0] main");
+    do_upgrade $MAJORS[0], $MAJORS[-1];
+    is ((system "pg_dropcluster $testversions[0] main"), 0, 'Dropping remaining cluster');
+} else {
+    pass 'only two available versions, skipping tests...';
+    for (my $i = 0; $i < 10; ++$i) {
+        pass '...';
+    }
+}
+
 check_clean;
 
 # vim: filetype=perl

Attachment: signature.asc
Description: Digital signature


Reply to: