Обсуждение: Rewriting the test of pg_upgrade as a TAP test - take two
Hi all, As promised on a recent thread, here is a second tentative to switch pg_upgrade's test.sh into a TAP infrastructure. This is a continuation of the following thread: https://www.postgresql.org/message-id/CAB7nPqRdaN1A1YNjxNL9T1jUEWct8ttqq29dNv8W_o37%2Be8wfA%40mail.gmail.com To begin with, compared to last version, I found and fixed a couple of bugs, and I also implemented an interface which allows for tests of pg_upgrade across major versions. The patch set is as follows: - 0001 refactors PostgresNode.pm so as a node's binary path is registered when created and can be enforced by the test creating the node via get_new_node. That's useful for pg_upgrade because we want to use different binary paths for the node to-be-upgraded and the new one. This is also useful for test suites willing to work on cross-version tests. - 0002 begins the move to a TAP suite by introducing a base set of tests for pg_upgrade. - 0003 is the real meat, and removes test.sh in favor of a TAP script aimed at supporting tests using the same version as well as cross-version tests. In order to do that, I have changed a bit the interface used for those tests. A use can enforce the following variables to run a test using a past version, while the new version is the one of the tree where the TAP script is kicked: export oldsrc=...somewhere/postgresql (old version's source tree) export oldbindir=...otherversion/bin (old version's installed bin dir) export oldlibdir=...otherversion/lib (old version's installed lib dir) make check That is documented as well in TESTING in the patch. While the patch is working great when working on the same major version, there is still an issue I am facing related to the loading of shared libraries in src/test/regress/ for the old server's version after upgrading it. Even by tweaking LD_LIBRARY_PATH I could not get the path load. The patch has been tweaked so as the objects from regression tests that depend on regress.so, autoinc.so and refint.so are not generated, allowing pg_upgrade to work, which is definitely wrong. I am sure that I missed a trick here but it is Friday here ;p Note also that the diff of the dump still needs to be eyeballed for two reasons: - hash index handling is different in v10~, as those exist now in the dumps taken. - PostgreSQL origin version string shows up. We could apply as well some post-filters to allow the test to pass, but that's one subject I would like to discuss on this thread. Another topic that I would like to discuss is how this interface is fit for the buildfarm code. After hacking my stuff, I have looked at the buildfarm code to notice that things like TestUpgradeXversion.pm do *not* make use of test.sh, and that what I hacked is much similar to what the buildfarm code is doing, which is itself roughly a copy of what test.sh does. Andrew, your feedback would be important here. So, thoughts? -- Michael
Вложения
Hi, On 2018-01-26 17:00:26 +0900, Michael Paquier wrote: > Another topic that I would like to discuss is how this interface is fit > for the buildfarm code. After hacking my stuff, I have looked at the > buildfarm code to notice that things like TestUpgradeXversion.pm do > *not* make use of test.sh, and that what I hacked is much similar to > what the buildfarm code is doing, which is itself roughly a copy of what > test.sh does. Andrew, your feedback would be important here. Andrew, any comments? > From 1294bb18426cea5c0764d0d07846fee291502b73 Mon Sep 17 00:00:00 2001 > From: Michael Paquier <michael@paquier.xyz> > Date: Wed, 24 Jan 2018 16:19:53 +0900 > Subject: [PATCH 1/3] Refactor PostgresNode.pm so as nodes can use custom > binary path > > This is a requirement for having a proper TAP test suite for pg_upgrade > where users have the possibility to manipulate nodes which use different > set of binaries during the upgrade processes. Seems reasonable. > + # Find binary directory for this new node. > + if (!defined($bindir)) > + { > + # If caller has not defined the binary directory, find it > + # from pg_config defined in this environment's PATH. > + my ($stdout, $stderr); > + my $result = IPC::Run::run [ 'pg_config', '--bindir' ], '>', > + \$stdout, '2>', \$stderr > + or die "could not execute pg_config"; > + chomp($stdout); > + $bindir = $stdout; > + } > + This isn't really required, we could just assume things are on PATH if bindir isn't specified. Don't have strong feelings about it. > From df74a70b886bb998ac37195b4d3067c41a012738 Mon Sep 17 00:00:00 2001 > From: Michael Paquier <michael@paquier.xyz> > Date: Wed, 24 Jan 2018 16:22:00 +0900 > Subject: [PATCH 2/3] Add basic TAP test setup for pg_upgrade > > The plan is to convert the current pg_upgrade test to the TAP > framework. This commit just puts a basic TAP test in place so that we > can see how the build farm behaves, since the build farm client has some > special knowledge of the pg_upgrade tests. Not sure I see the point of keeping this separate, but whatever... > From f903e01bbb65f1e38cd74b01ee99c4526e4c3b7f Mon Sep 17 00:00:00 2001 > From: Michael Paquier <michael@paquier.xyz> > Date: Fri, 26 Jan 2018 16:37:42 +0900 > Subject: [PATCH 3/3] Replace pg_upgrade's test.sh by a TAP test > > This replacement still allows tests across major versions, though the > interface to reach that has been changed a bit. See TESTING for more > details on the matter. > +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl > @@ -0,0 +1,213 @@ > +# Set of tests for pg_upgrade. > +use strict; > +use warnings; > +use Cwd; > +use Config; > +use File::Basename; > +use File::Copy; > +use IPC::Run; > +use PostgresNode; > +use TestLib; > +use Test::More tests => 4; > + > +# Generate a database with a name made of a range of ASCII characters. > +sub generate_db > +{ s/_/_random_/? > +# From now on, Odd phrasing imo. > > +elsif (defined($ENV{oldsrc}) && > + defined($ENV{oldbindir}) && > + defined($ENV{oldlibdir})) > +{ > + # A run is wanted on an old version as base. > + $oldsrc = $ENV{oldsrc}; > + $oldbindir = $ENV{oldbindir}; > + $oldlibdir = $ENV{oldlibdir}; > + # FIXME: this needs better tuning. Using "." or "$oldlibdir/postgresql" > + # causes the regression tests to pass but pg_upgrade to fail afterwards. Planning to fix it? > +# Install manually regress.so, this got forgotten in the process. > +copy "$oldsrc/src/test/regress/regress.so", > + "$oldlibdir/postgresql/regress.so" > + unless (-e "$oldlibdir/regress.so"); Weird comment again. > +# Run regression tests on the old instance, using the binaries of this > +# instance. At the same time create a tablespace path needed for the > +# tests, similarly to what "make check" creates. What does "using binaries of this instance" mean? And why? > +chdir dirname($regress_bin); > +rmdir "testtablespace"; > +mkdir "testtablespace"; > +$oldnode->run_log([ "$oldbindir/createdb", '--port', $oldnode->port, > + 'regression' ]); > +$oldnode->command_ok([$regress_bin, '--schedule', './serial_schedule', > + '--dlpath', "$dlpath", '--bindir', $oldnode->bin_dir, > + '--use-existing', '--port', $oldnode->port], > + 'regression test run on old instance'); > +# Before dumping, get rid of objects not existing in later versions. This > +# depends on the version of the old server used, and matters only if the > +# old and new source paths > +my $oldpgversion; > +($result, $oldpgversion, $stderr) = > + $oldnode->psql('postgres', qq[SHOW server_version_num;]); > +my $fix_sql; > +if ($newsrc ne $oldsrc) > +{ > + if ($oldpgversion <= 80400) > + { > + $fix_sql = "DROP FUNCTION public.myfunc(integer); DROP FUNCTION public.oldstyle_length(integer, text);"; > + } > + else > + { > + $fix_sql = "DROP FUNCTION public.oldstyle_length(integer, text);"; > + } > + $oldnode->psql('postgres', $fix_sql); > +} I know you copied this, but what? > +# Take a dump before performing the upgrade as a base comparison. Note > +# that we need to use pg_dumpall from PATH here. Whe do we need to? Greetings, Andres Freund
On Fri, Mar 2, 2018 at 4:38 PM, Andres Freund <andres@anarazel.de> wrote: > Hi, > > On 2018-01-26 17:00:26 +0900, Michael Paquier wrote: >> Another topic that I would like to discuss is how this interface is fit >> for the buildfarm code. After hacking my stuff, I have looked at the >> buildfarm code to notice that things like TestUpgradeXversion.pm do >> *not* make use of test.sh, and that what I hacked is much similar to >> what the buildfarm code is doing, which is itself roughly a copy of what >> test.sh does. Andrew, your feedback would be important here. > > Andrew, any comments? > > I'll take a look. Meanwhile: TestUpgradeXversion.pm is different in a number of respects from the inbuilt test regime. It doesn't run the normal regression suite to set up a test. Rather, it saves out the installed binaries and data directory of the buildfarm client, and then tries to upgrade those saved data directorories from earlier branches or the current branch to the target version being tested. It has to make a few adjustments to the databases along the way. It does a compare of pre- and post- pg_dumpall runs, allowing a fuzz factor if the versions are different. The other upside to the scheme is that we're testing pg_dump against earlier branches as part of testing pg_upgrade. All this consumes quite a bit of disk space - currently 3.5Gb between runs on crake. That could probably be reduced some by removing log as we go. I don't think a scheme like this is going to be terribly workable outside some system such as the buildfarm that deals with multiple branches. One of the significant pluses to TestUpgradeXversion.pm is that it tests upgrading quite a bit more than the standard regression database. It also tests all the contrib databases and the isolation and pl_regression databases. Several bugs have been found that way, IIRC, and we should arguably do something along the same lines for our builtin testing. I'll post a follow up when I've had a chance to have a good look at what Michael has actually done. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Mar 01, 2018 at 10:08:06PM -0800, Andres Freund wrote: > On 2018-01-26 17:00:26 +0900, Michael Paquier wrote: >> +elsif (defined($ENV{oldsrc}) && >> + defined($ENV{oldbindir}) && >> + defined($ENV{oldlibdir})) >> +{ >> + # A run is wanted on an old version as base. >> + $oldsrc = $ENV{oldsrc}; >> + $oldbindir = $ENV{oldbindir}; >> + $oldlibdir = $ENV{oldlibdir}; >> + # FIXME: this needs better tuning. Using "." or "$oldlibdir/postgresql" >> + # causes the regression tests to pass but pg_upgrade to fail afterwards. > > Planning to fix it? This is something I am not completely sure yet how to move on with. Those areas would need adjustment once we ave a better idea what the buildfarm can make use of. >> +# Run regression tests on the old instance, using the binaries of this >> +# instance. At the same time create a tablespace path needed for the >> +# tests, similarly to what "make check" creates. > > What does "using binaries of this instance" mean? And why? This refers to the installation of the instance to upgrade, including pg_regress installed in pgxs. >> +# Before dumping, get rid of objects not existing in later versions. This >> +# depends on the version of the old server used, and matters only if the >> +# old and new source paths >> +my $oldpgversion; >> +($result, $oldpgversion, $stderr) = >> + $oldnode->psql('postgres', qq[SHOW server_version_num;]); >> +my $fix_sql; >> +if ($newsrc ne $oldsrc) >> +{ >> + if ($oldpgversion <= 80400) >> + { >> + $fix_sql = "DROP FUNCTION public.myfunc(integer); DROP FUNCTION public.oldstyle_length(integer, text);"; >> + } >> + else >> + { >> + $fix_sql = "DROP FUNCTION public.oldstyle_length(integer, text);"; >> + } >> + $oldnode->psql('postgres', $fix_sql); >> +} > > I know you copied this, but what? Doesn't it matter to be able to test cross upgrades where the instance to upgrade is 8.4? >> +# Take a dump before performing the upgrade as a base comparison. Note >> +# that we need to use pg_dumpall from PATH here. > > Whe do we need to? Yeah, this should refer to the pg_dumpall command from the new instance. -- Michael
Вложения
On Fri, Mar 02, 2018 at 06:33:55PM +1030, Andrew Dunstan wrote: > TestUpgradeXversion.pm is different in a number of respects from the > inbuilt test regime. It doesn't run the normal regression suite to set > up a test. Rather, it saves out the installed binaries and data > directory of the buildfarm client, and then tries to upgrade those > saved data directorories from earlier branches or the current branch > to the target version being tested. It has to make a few adjustments > to the databases along the way. It does a compare of pre- and post- > pg_dumpall runs, allowing a fuzz factor if the versions are different. > The other upside to the scheme is that we're testing pg_dump against > earlier branches as part of testing pg_upgrade. Thanks for the details. test.sh also does a comparison of the output of pg_dumpall between the instance before and after upgrade. The adjustments done are also close to what test.sh does, and what my patch does. This consists in update prosrc for defined functions (+alpha), right? > I don't think a scheme like this is going to be terribly workable > outside some system such as the buildfarm that deals with multiple > branches. Yeah, you need the code tree of the past instance as well as the regression tests need to be run on the instance to upgrade. > One of the significant pluses to TestUpgradeXversion.pm is that it > tests upgrading quite a bit more than the standard regression > database. It also tests all the contrib databases and the isolation > and pl_regression databases. Several bugs have been found that way, > IIRC, and we should arguably do something along the same lines for our > builtin testing. You have a point here. I did not consider those parts. > I'll post a follow up when I've had a chance to have a good look at > what Michael has actually done. I was thinking about that a bit today, and a nice goal would be to come up with an infrastructure that could be shared between the buildfarm and the in-core code. Roughly I think that what I developed could be changed so as we have TestUpgradeXversion.pm, which the buildfarm could use, and the in-core TAP tests would be a thin wrapper on top of it. I doubt also a bit how much people actually use test.sh to run cross-version upgrades.. 5bab1985 is teaching this lesson. -- Michael
Вложения
Andrew Dunstan wrote: > One of the significant pluses to TestUpgradeXversion.pm is that it > tests upgrading quite a bit more than the standard regression > database. It also tests all the contrib databases and the isolation > and pl_regression databases. I think we should definitely be including the src/bin/pg_dump/ test database in this set of tests (if it isn't already), since that one is supposed to include comprehensive object type coverage. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/26/18 03:00, Michael Paquier wrote: > As promised on a recent thread, here is a second tentative to switch > pg_upgrade's test.sh into a TAP infrastructure. AFAICT, this still has the same problem as the previous take, namely that adding a TAP test suite to the pg_upgrade subdirectory will end up with the build farm client running the pg_upgrade tests twice. What we likely need here is an update to the build farm client in conjunction with this. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Mar 4, 2018 at 12:42 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 1/26/18 03:00, Michael Paquier wrote: >> As promised on a recent thread, here is a second tentative to switch >> pg_upgrade's test.sh into a TAP infrastructure. > > AFAICT, this still has the same problem as the previous take, namely > that adding a TAP test suite to the pg_upgrade subdirectory will end up > with the build farm client running the pg_upgrade tests twice. What we > likely need here is an update to the build farm client in conjunction > with this. > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services > Something like this should do the trick. diff --git a/PGBuild/Modules/TestUpgrade.pm b/PGBuild/Modules/TestUpgrade.pm index 8406d27..05859f9 100644 --- a/PGBuild/Modules/TestUpgrade.pm +++ b/PGBuild/Modules/TestUpgrade.pm @@ -50,6 +50,11 @@ sub setup "overly long build root $buildroot will cause upgrade problems - try something shorter than 46 chars" if (length($buildroot) > 46); + # don't run module if builtin test is enabled. + my $using_tap_tests = $conf->using_msvc ? $conf->{config}->{tap_tests} : + grep {$_ eq '--enable-tap-tests'} @{$conf->{config}} ; + return if $using_tap_tests && -d "$buildroot/$branch/pgsql/src/bin/pg_upgrade/t"; + # could even set up several of these (e.g. for different branches) my $self = { buildroot => $buildroot, cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Mar 4, 2018 at 2:34 PM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: > On Sun, Mar 4, 2018 at 12:42 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> On 1/26/18 03:00, Michael Paquier wrote: >>> As promised on a recent thread, here is a second tentative to switch >>> pg_upgrade's test.sh into a TAP infrastructure. >> >> AFAICT, this still has the same problem as the previous take, namely >> that adding a TAP test suite to the pg_upgrade subdirectory will end up >> with the build farm client running the pg_upgrade tests twice. What we >> likely need here is an update to the build farm client in conjunction >> with this. >> >> -- >> Peter Eisentraut http://www.2ndQuadrant.com/ >> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >> > > Something like this should do the trick. [ tiny patch] Pushed with a bug fix. See <https://github.com/PGBuildFarm/client-code/commit/826d450eff05d15c8bb3c5b2728da5328634a588> If you want to do this soon I can put out a Buildfarm Client release fairly quickly. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/4/18 16:09, Andrew Dunstan wrote: >>> AFAICT, this still has the same problem as the previous take, namely >>> that adding a TAP test suite to the pg_upgrade subdirectory will end up >>> with the build farm client running the pg_upgrade tests twice. What we >>> likely need here is an update to the build farm client in conjunction >>> with this. > Pushed with a bug fix. See > <https://github.com/PGBuildFarm/client-code/commit/826d450eff05d15c8bb3c5b2728da5328634a588> > > If you want to do this soon I can put out a Buildfarm Client release > fairly quickly. I think the dependency is mostly the other way around. How quickly would build farm owners install the upgrade? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > On 3/4/18 16:09, Andrew Dunstan wrote: >> If you want to do this soon I can put out a Buildfarm Client release >> fairly quickly. > I think the dependency is mostly the other way around. How quickly > would build farm owners install the upgrade? IIUC, the buildfarm script patch is only needed to avoid duplicate tests. So owners need only install it if they want to reduce wasted cycles on their machines. That being the case, it's only urgent to the extent that the individual owner perceives it to be. Some might think it is so, so I'd like to see the BF release available before we push the TAP test ... but we don't have to wait very long between. regards, tom lane
On 3/6/18 4:12 PM, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> On 3/4/18 16:09, Andrew Dunstan wrote: >>> If you want to do this soon I can put out a Buildfarm Client release >>> fairly quickly. > >> I think the dependency is mostly the other way around. How quickly >> would build farm owners install the upgrade? > > IIUC, the buildfarm script patch is only needed to avoid duplicate > tests. So owners need only install it if they want to reduce wasted > cycles on their machines. That being the case, it's only urgent to > the extent that the individual owner perceives it to be. Some might > think it is so, so I'd like to see the BF release available before > we push the TAP test ... but we don't have to wait very long between. It seems the consensus is that we'll need a build farm update before we can move forward with the patch and that we don't need to wait long for people to upgrade. Andrew, do you have a date for the next release? Thanks, -- -David david@pgmasters.net
On Wed, Mar 21, 2018 at 10:51:04AM -0400, David Steele wrote: > On 3/6/18 4:12 PM, Tom Lane wrote: > It seems the consensus is that we'll need a build farm update before we > can move forward with the patch and that we don't need to wait long for > people to upgrade. > > Andrew, do you have a date for the next release? Even if a lighter version of this patch stripped from multi-version support (Who uses that actually?) knowing that the buildfarm code has its own set of modules to do the same may be a good step forward as per the feedback of this thread people would like to unify the existing scripts. We could also try to figure later on how to merge the buildfarm code into upstream and if it actually makes sense or not, which is something I am not completely convinced is worth it. This would remove the need of any complicated refactoring in PostgresNode.pm to handle binary paths for a single node, at the cost of of course removing cross-upgrade checks from the tree. Still I definitely agree that a release of the buildfarm client should happen first. 9 days before the end of the commit, the window may be at the end too short for v11. -- Michael
Вложения
On 22 March 2018 at 09:39, Michael Paquier <michael@paquier.xyz> wrote:
-- On Wed, Mar 21, 2018 at 10:51:04AM -0400, David Steele wrote:
> On 3/6/18 4:12 PM, Tom Lane wrote:
> It seems the consensus is that we'll need a build farm update before we
> can move forward with the patch and that we don't need to wait long for
> people to upgrade.
>
> Andrew, do you have a date for the next release?
Even if a lighter version of this patch stripped from multi-version
support (Who uses that actually?)
I'm super excited by the idea of multi-version support in TAP, if that's what you mean.
Why? Because I use TAP heavily in extensions. Especially replication extensions. Which like to talk across multiple versions. I currently need external test frameworks and some hideous hacks to do this.
On Thu, Mar 22, 2018 at 09:42:35AM +0800, Craig Ringer wrote: > I'm super excited by the idea of multi-version support in TAP, if that's > what you mean. > > Why? Because I use TAP heavily in extensions. Especially replication > extensions. Which like to talk across multiple versions. I currently need > external test frameworks and some hideous hacks to do this. Okay, in front of such enthusiasm we could keep at least the refactoring part of PostgresNode.pm :) -- Michael
Вложения
On 3/21/18 21:49, Michael Paquier wrote: > On Thu, Mar 22, 2018 at 09:42:35AM +0800, Craig Ringer wrote: >> I'm super excited by the idea of multi-version support in TAP, if that's >> what you mean. >> >> Why? Because I use TAP heavily in extensions. Especially replication >> extensions. Which like to talk across multiple versions. I currently need >> external test frameworks and some hideous hacks to do this. > > Okay, in front of such enthusiasm we could keep at least the refactoring > part of PostgresNode.pm :) I took a quick look at that part. It appears to be quite invasive, more than I would have hoped. Basically, it imposes that from now on all program invocations must observe the bindir setting, which someone is surely going to forget. The pg_upgrade tests aren't going to exercise all possible paths where programs are called, so this is going to lead to omissions and inconsistencies -- which will then possibly only be found much later by the extensions that Craig was talking about. I'd like to see this more isolated, maybe via a path change, or something inside system_or_bail or something less invasive and more maintainable. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Apr 02, 2018 at 10:35:09AM -0400, Peter Eisentraut wrote: > I took a quick look at that part. It appears to be quite invasive, more > than I would have hoped. Basically, it imposes that from now on all > program invocations must observe the bindir setting, which someone is > surely going to forget. The pg_upgrade tests aren't going to exercise > all possible paths where programs are called, so this is going to lead > to omissions and inconsistencies -- which will then possibly only be > found much later by the extensions that Craig was talking about. I'd > like to see this more isolated, maybe via a path change, or something > inside system_or_bail or something less invasive and more maintainable. Hm. PostgresNode.pm uses now a couple of different ways to trigger commands, which makes the problem harder than it seems: - system_or_bail - system_log - IPC::Run::timeout - IPC::Run::run Because of that, and if one wants to minimize the number of places where bindir is called, would be to modify the command strings themselves. What I could think of would be to have a wrapper like build_bin_path which adds $bindir on top of the binary, but that would still cause all the callers to use this wrapper. The use of this wrapper could still be forgotten. Enforcing PATH temporarily in a code path or another implies that the previous value of PATH needs to be added back, which could be forgotten as well. At the end, it seems to me that what I am proposing is themost readable approach, and with proper documentation things should be handled finely... Or there is an approach you have in mind I do not foresee? -- Michael
Вложения
On Tue, Apr 03, 2018 at 03:04:10PM +0900, Michael Paquier wrote: > At the end, it seems to me that what I am proposing is themost readable > approach, and with proper documentation things should be handled > finely... Or there is an approach you have in mind I do not foresee? With feature freeze which has been reached, I am marking this patch as returned with feedback. Please note that I have no plans to resubmit again this patch set after this second attempt as I have no idea where we want things to go. -- Michael