[OSSTEST PATCH v2 0/8] Support database schema updates

[OSSTEST PATCH v2 0/8] Support database schema updates

[Ian Jackson]

Thanks for the quick review.  There is little change here, other than
significant changes to the README in
   5/8  Schema: Support database schema updates
as prompted by review comments.

All the patches apart from that and
   4/8  Schema: Introduce mg-schema-create
have been acked.

Thanks,
Ian.

[OSSTEST PATCH 1/8] mg-schema-test-database: Fix argument parsing for _SUFFIX

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 mg-schema-test-database |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index d704950..9791d84 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -244,7 +244,7 @@ create)
 				-v type="${rhs%% *}"		\
 				-v refkey="${rhs#* }"
 			;;
-		_)	suffix="$arg"
+		_*)	suffix="$arg"
 			;;
 		-f*)	minflight="${arg#-f}"
 			;;
-- 
1.7.10.4

[OSSTEST PATCH 2/8] Schema: Rename schema file

[Ian Jackson]

We are going to have multiple schema snippets and this is going be
just the initial baseline.

Rename the file and change references to it.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 INSTALL.production                                |    4 ++--
 mg-schema-test-database                           |    2 +-
 executive-postgresql-schema => schema/initial.sql |    0
 3 files changed, 3 insertions(+), 3 deletions(-)
 rename executive-postgresql-schema => schema/initial.sql (100%)

diff --git a/INSTALL.production b/INSTALL.production
index 2db8bd3..2114ccc 100644
--- a/INSTALL.production
+++ b/INSTALL.production
@@ -29,8 +29,8 @@ DATABASE SERVER
 ---------------
 
 osstest requires a Postgres database server and a database configured
-with the schema described in executive-postgresql-schema which should
-be accessible to an osstest role account.
+with the schema described in schema/initial.sql should be accessible
+to an osstest role account.
 
 The hostname and dbname are configured via the "ExecutiveDbnamePat"
 config option.
diff --git a/mg-schema-test-database b/mg-schema-test-database
index 9791d84..0c4dab7 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -387,7 +387,7 @@ END
 	tables=$(tsort <$t.tablesortlist)
 
 	# We don't want to set the permissions
-	perl <executive-postgresql-schema >$t.new-schema -pe '
+	perl <schema/initial.sql >$t.new-schema -pe '
 		s/^/--/ if
 			m/^ALTER TABLE .* OWNER TO / ||
 			m/^GRANT |^REVOKE /
diff --git a/executive-postgresql-schema b/schema/initial.sql
similarity index 100%
rename from executive-postgresql-schema
rename to schema/initial.sql
-- 
1.7.10.4

[OSSTEST PATCH 3/8] Schema: Remove SET OWNER and GRANT/REVOKE from schema/initial.sql

[Ian Jackson]

Really, we don't want the initial schema setup to mess about with
permissions.  Instead, we simply expect to run the creation as the
correct role user.

So:
 - Remove the code in mg-schema-test-database to remove the
   permission settings from initial.sql;
 - Instead, run exactly that code on initial.sql and commit the
   result.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 mg-schema-test-database |    9 +---
 schema/initial.sql      |  132 +++++++++++++++++++++++------------------------
 2 files changed, 67 insertions(+), 74 deletions(-)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index 0c4dab7..c68b1d2 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -386,19 +386,12 @@ END
 
 	tables=$(tsort <$t.tablesortlist)
 
-	# We don't want to set the permissions
-	perl <schema/initial.sql >$t.new-schema -pe '
-		s/^/--/ if
-			m/^ALTER TABLE .* OWNER TO / ||
-			m/^GRANT |^REVOKE /
-	'
-
 	#---------- create test db ----------
 
 	psql_do <<END
 		CREATE DATABASE $dbname;
 END
-	$(withtest get_psql_cmd) -q -f $t.new-schema
+	$(withtest get_psql_cmd) -q -f schema/initial.sql
 
 	printf ".\n"
 
diff --git a/schema/initial.sql b/schema/initial.sql
index 7bd6c55..4feeeb2 100644
--- a/schema/initial.sql
+++ b/schema/initial.sql
@@ -28,7 +28,7 @@ CREATE TABLE flights (
 );
 
 
-ALTER TABLE public.flights OWNER TO osstest;
+--ALTER TABLE public.flights OWNER TO osstest;
 
 --
 -- Name: flights_flight_seq; Type: SEQUENCE; Schema: public; Owner: osstest
@@ -42,7 +42,7 @@ CREATE SEQUENCE flights_flight_seq
     CACHE 1;
 
 
-ALTER TABLE public.flights_flight_seq OWNER TO osstest;
+--ALTER TABLE public.flights_flight_seq OWNER TO osstest;
 
 --
 -- Name: flights_flight_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: osstest
@@ -61,7 +61,7 @@ CREATE TABLE flights_harness_touched (
 );
 
 
-ALTER TABLE public.flights_harness_touched OWNER TO osstest;
+--ALTER TABLE public.flights_harness_touched OWNER TO osstest;
 
 --
 -- Name: hostflags; Type: TABLE; Schema: public; Owner: osstest; Tablespace: 
@@ -73,7 +73,7 @@ CREATE TABLE hostflags (
 );
 
 
-ALTER TABLE public.hostflags OWNER TO osstest;
+--ALTER TABLE public.hostflags OWNER TO osstest;
 
 --
 -- Name: jobs; Type: TABLE; Schema: public; Owner: osstest; Tablespace: 
@@ -87,7 +87,7 @@ CREATE TABLE jobs (
 );
 
 
-ALTER TABLE public.jobs OWNER TO osstest;
+--ALTER TABLE public.jobs OWNER TO osstest;
 
 --
 -- Name: resource_log_evid_seq; Type: SEQUENCE; Schema: public; Owner: iwj
@@ -101,7 +101,7 @@ CREATE SEQUENCE resource_log_evid_seq
     CACHE 1;
 
 
-ALTER TABLE public.resource_log_evid_seq OWNER TO iwj;
+--ALTER TABLE public.resource_log_evid_seq OWNER TO iwj;
 
 --
 -- Name: resource_log; Type: TABLE; Schema: public; Owner: iwj; Tablespace: 
@@ -122,7 +122,7 @@ CREATE TABLE resource_log (
 );
 
 
-ALTER TABLE public.resource_log OWNER TO iwj;
+--ALTER TABLE public.resource_log OWNER TO iwj;
 
 --
 -- Name: resource_properties; Type: TABLE; Schema: public; Owner: osstest; Tablespace: 
@@ -136,7 +136,7 @@ CREATE TABLE resource_properties (
 );
 
 
-ALTER TABLE public.resource_properties OWNER TO osstest;
+--ALTER TABLE public.resource_properties OWNER TO osstest;
 
 --
 -- Name: resource_sharing; Type: TABLE; Schema: public; Owner: osstest; Tablespace: 
@@ -152,7 +152,7 @@ CREATE TABLE resource_sharing (
 );
 
 
-ALTER TABLE public.resource_sharing OWNER TO osstest;
+--ALTER TABLE public.resource_sharing OWNER TO osstest;
 
 --
 -- Name: resources; Type: TABLE; Schema: public; Owner: osstest; Tablespace: 
@@ -168,7 +168,7 @@ CREATE TABLE resources (
 );
 
 
-ALTER TABLE public.resources OWNER TO osstest;
+--ALTER TABLE public.resources OWNER TO osstest;
 
 --
 -- Name: runvars; Type: TABLE; Schema: public; Owner: osstest; Tablespace: 
@@ -183,7 +183,7 @@ CREATE TABLE runvars (
 );
 
 
-ALTER TABLE public.runvars OWNER TO osstest;
+--ALTER TABLE public.runvars OWNER TO osstest;
 
 --
 -- Name: steps; Type: TABLE; Schema: public; Owner: osstest; Tablespace: 
@@ -201,7 +201,7 @@ CREATE TABLE steps (
 );
 
 
-ALTER TABLE public.steps OWNER TO osstest;
+--ALTER TABLE public.steps OWNER TO osstest;
 
 --
 -- Name: tasks_taskid_seq; Type: SEQUENCE; Schema: public; Owner: osstest
@@ -215,7 +215,7 @@ CREATE SEQUENCE tasks_taskid_seq
     CACHE 1;
 
 
-ALTER TABLE public.tasks_taskid_seq OWNER TO osstest;
+--ALTER TABLE public.tasks_taskid_seq OWNER TO osstest;
 
 --
 -- Name: tasks; Type: TABLE; Schema: public; Owner: osstest; Tablespace: 
@@ -232,7 +232,7 @@ CREATE TABLE tasks (
 );
 
 
-ALTER TABLE public.tasks OWNER TO osstest;
+--ALTER TABLE public.tasks OWNER TO osstest;
 
 --
 -- Name: flight; Type: DEFAULT; Schema: public; Owner: osstest
@@ -409,130 +409,130 @@ ALTER TABLE ONLY steps
 -- Name: public; Type: ACL; Schema: -; Owner: postgres
 --
 
-REVOKE ALL ON SCHEMA public FROM PUBLIC;
-REVOKE ALL ON SCHEMA public FROM postgres;
-GRANT ALL ON SCHEMA public TO postgres;
-GRANT ALL ON SCHEMA public TO PUBLIC;
+--REVOKE ALL ON SCHEMA public FROM PUBLIC;
+--REVOKE ALL ON SCHEMA public FROM postgres;
+--GRANT ALL ON SCHEMA public TO postgres;
+--GRANT ALL ON SCHEMA public TO PUBLIC;
 
 
 --
 -- Name: flights; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON TABLE flights FROM PUBLIC;
-REVOKE ALL ON TABLE flights FROM osstest;
-GRANT ALL ON TABLE flights TO osstest;
-GRANT SELECT ON TABLE flights TO osstest_ro;
+--REVOKE ALL ON TABLE flights FROM PUBLIC;
+--REVOKE ALL ON TABLE flights FROM osstest;
+--GRANT ALL ON TABLE flights TO osstest;
+--GRANT SELECT ON TABLE flights TO osstest_ro;
 
 
 --
 -- Name: flights_flight_seq; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON SEQUENCE flights_flight_seq FROM PUBLIC;
-REVOKE ALL ON SEQUENCE flights_flight_seq FROM osstest;
-GRANT ALL ON SEQUENCE flights_flight_seq TO osstest;
-GRANT SELECT ON SEQUENCE flights_flight_seq TO osstest_ro;
+--REVOKE ALL ON SEQUENCE flights_flight_seq FROM PUBLIC;
+--REVOKE ALL ON SEQUENCE flights_flight_seq FROM osstest;
+--GRANT ALL ON SEQUENCE flights_flight_seq TO osstest;
+--GRANT SELECT ON SEQUENCE flights_flight_seq TO osstest_ro;
 
 
 --
 -- Name: flights_harness_touched; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON TABLE flights_harness_touched FROM PUBLIC;
-REVOKE ALL ON TABLE flights_harness_touched FROM osstest;
-GRANT ALL ON TABLE flights_harness_touched TO osstest;
-GRANT SELECT ON TABLE flights_harness_touched TO osstest_ro;
+--REVOKE ALL ON TABLE flights_harness_touched FROM PUBLIC;
+--REVOKE ALL ON TABLE flights_harness_touched FROM osstest;
+--GRANT ALL ON TABLE flights_harness_touched TO osstest;
+--GRANT SELECT ON TABLE flights_harness_touched TO osstest_ro;
 
 
 --
 -- Name: hostflags; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON TABLE hostflags FROM PUBLIC;
-REVOKE ALL ON TABLE hostflags FROM osstest;
-GRANT ALL ON TABLE hostflags TO osstest;
-GRANT SELECT ON TABLE hostflags TO osstest_ro;
+--REVOKE ALL ON TABLE hostflags FROM PUBLIC;
+--REVOKE ALL ON TABLE hostflags FROM osstest;
+--GRANT ALL ON TABLE hostflags TO osstest;
+--GRANT SELECT ON TABLE hostflags TO osstest_ro;
 
 
 --
 -- Name: jobs; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON TABLE jobs FROM PUBLIC;
-REVOKE ALL ON TABLE jobs FROM osstest;
-GRANT ALL ON TABLE jobs TO osstest;
-GRANT SELECT ON TABLE jobs TO osstest_ro;
+--REVOKE ALL ON TABLE jobs FROM PUBLIC;
+--REVOKE ALL ON TABLE jobs FROM osstest;
+--GRANT ALL ON TABLE jobs TO osstest;
+--GRANT SELECT ON TABLE jobs TO osstest_ro;
 
 
 --
 -- Name: resource_properties; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON TABLE resource_properties FROM PUBLIC;
-REVOKE ALL ON TABLE resource_properties FROM osstest;
-GRANT ALL ON TABLE resource_properties TO osstest;
-GRANT SELECT ON TABLE resource_properties TO osstest_ro;
+--REVOKE ALL ON TABLE resource_properties FROM PUBLIC;
+--REVOKE ALL ON TABLE resource_properties FROM osstest;
+--GRANT ALL ON TABLE resource_properties TO osstest;
+--GRANT SELECT ON TABLE resource_properties TO osstest_ro;
 
 
 --
 -- Name: resource_sharing; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON TABLE resource_sharing FROM PUBLIC;
-REVOKE ALL ON TABLE resource_sharing FROM osstest;
-GRANT ALL ON TABLE resource_sharing TO osstest;
-GRANT SELECT ON TABLE resource_sharing TO osstest_ro;
+--REVOKE ALL ON TABLE resource_sharing FROM PUBLIC;
+--REVOKE ALL ON TABLE resource_sharing FROM osstest;
+--GRANT ALL ON TABLE resource_sharing TO osstest;
+--GRANT SELECT ON TABLE resource_sharing TO osstest_ro;
 
 
 --
 -- Name: resources; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON TABLE resources FROM PUBLIC;
-REVOKE ALL ON TABLE resources FROM osstest;
-GRANT ALL ON TABLE resources TO osstest;
-GRANT SELECT ON TABLE resources TO osstest_ro;
+--REVOKE ALL ON TABLE resources FROM PUBLIC;
+--REVOKE ALL ON TABLE resources FROM osstest;
+--GRANT ALL ON TABLE resources TO osstest;
+--GRANT SELECT ON TABLE resources TO osstest_ro;
 
 
 --
 -- Name: runvars; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON TABLE runvars FROM PUBLIC;
-REVOKE ALL ON TABLE runvars FROM osstest;
-GRANT ALL ON TABLE runvars TO osstest;
-GRANT SELECT ON TABLE runvars TO osstest_ro;
+--REVOKE ALL ON TABLE runvars FROM PUBLIC;
+--REVOKE ALL ON TABLE runvars FROM osstest;
+--GRANT ALL ON TABLE runvars TO osstest;
+--GRANT SELECT ON TABLE runvars TO osstest_ro;
 
 
 --
 -- Name: steps; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON TABLE steps FROM PUBLIC;
-REVOKE ALL ON TABLE steps FROM osstest;
-GRANT ALL ON TABLE steps TO osstest;
-GRANT SELECT ON TABLE steps TO osstest_ro;
+--REVOKE ALL ON TABLE steps FROM PUBLIC;
+--REVOKE ALL ON TABLE steps FROM osstest;
+--GRANT ALL ON TABLE steps TO osstest;
+--GRANT SELECT ON TABLE steps TO osstest_ro;
 
 
 --
 -- Name: tasks_taskid_seq; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON SEQUENCE tasks_taskid_seq FROM PUBLIC;
-REVOKE ALL ON SEQUENCE tasks_taskid_seq FROM osstest;
-GRANT ALL ON SEQUENCE tasks_taskid_seq TO osstest;
-GRANT SELECT ON SEQUENCE tasks_taskid_seq TO osstest_ro;
+--REVOKE ALL ON SEQUENCE tasks_taskid_seq FROM PUBLIC;
+--REVOKE ALL ON SEQUENCE tasks_taskid_seq FROM osstest;
+--GRANT ALL ON SEQUENCE tasks_taskid_seq TO osstest;
+--GRANT SELECT ON SEQUENCE tasks_taskid_seq TO osstest_ro;
 
 
 --
 -- Name: tasks; Type: ACL; Schema: public; Owner: osstest
 --
 
-REVOKE ALL ON TABLE tasks FROM PUBLIC;
-REVOKE ALL ON TABLE tasks FROM osstest;
-GRANT ALL ON TABLE tasks TO osstest;
-GRANT SELECT ON TABLE tasks TO osstest_ro;
+--REVOKE ALL ON TABLE tasks FROM PUBLIC;
+--REVOKE ALL ON TABLE tasks FROM osstest;
+--GRANT ALL ON TABLE tasks TO osstest;
+--GRANT SELECT ON TABLE tasks TO osstest_ro;
 
 
 --
-- 
1.7.10.4

[OSSTEST PATCH 4/8] Schema: Introduce mg-schema-create

[Ian Jackson]

There is a fair amount of option parsing clobber here that will be
relevant shortly.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 mg-schema-create        |   61 +++++++++++++++++++++++++++++++++++++++++++++++
 mg-schema-test-database |    2 +-
 2 files changed, 62 insertions(+), 1 deletion(-)
 create mode 100755 mg-schema-create

diff --git a/mg-schema-create b/mg-schema-create
new file mode 100755
index 0000000..54f1c76
--- /dev/null
+++ b/mg-schema-create
@@ -0,0 +1,61 @@
+#!/bin/bash
+
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 2009-2015 Citrix Inc.
+# 
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+# 
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+# Usages:
+#
+#  ./mg-schema-create [<options>]
+#
+#  Database must already exist.  (Ie, mg-schema-create does not
+#  do CREATE DATABASE.)
+#
+#  When setting up a production database, mg-schema-create should
+#  be run *AS THE ROLE USER* who is to own all the resources.
+#
+# Options:
+#
+#  -q                            don't print progress messages
+
+set -e
+set -o posix
+set -o pipefail
+
+progress () { printf "%s\n" "$*"; }
+progress=progress
+quietopt=''
+
+while [ $# != 0 ]; do
+    arg=$1; shift
+    case "$arg" in
+    -q)
+        progress=:
+        quietopt=-q
+        ;;
+    *)
+        echo >&2 "bad usage ($arg)"; exit 127
+        ;;
+    esac
+done
+
+. ./cri-getconfig
+
+$progress "Populating database..."
+
+$(get_psql_cmd) $quietopt -f schema/initial.sql
+
+$progress "Database set up."
diff --git a/mg-schema-test-database b/mg-schema-test-database
index c68b1d2..3616c4d 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -391,7 +391,7 @@ END
 	psql_do <<END
 		CREATE DATABASE $dbname;
 END
-	$(withtest get_psql_cmd) -q -f schema/initial.sql
+	withtest ./mg-schema-create -q
 
 	printf ".\n"
 
-- 
1.7.10.4

Re: [OSSTEST PATCH 4/8] Schema: Introduce mg-schema-create

[Ian Campbell]

On Thu, 2015-12-10 at 17:12 +0000, Ian Jackson wrote:
> There is a fair amount of option parsing clobber here that will be
> relevant shortly.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(perhaps assuming #8 goes in in the same batch)

Re: [OSSTEST PATCH 4/8] Schema: Introduce mg-schema-create

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 4/8] Schema: Introduce mg-schema-create"):
> On Thu, 2015-12-10 at 17:12 +0000, Ian Jackson wrote:
> > There is a fair amount of option parsing clobber here that will be
> > relevant shortly.
> > 
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> (perhaps assuming #8 goes in in the same batch)

It should do.

Ian.

[OSSTEST PATCH 5/8] Schema: Support database schema updates

[Ian Jackson]

See schema/README.schema, introduced in this patch, for the design.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: Slight increase schema update name length format.
    Docs fixes:
    Change erroneous `three' to `four'.
    Change `state' to `status' throghout.
    Explain scope of <status>.
    Sort out (and renumber) `Update order for Populate-then-rely'.
    Sort out "Statuses" explanations.
    Encourage use of DML update, rather than ad-hoc scripts,
     for populating new columns.
---
 Osstest/Executive.pm      |   82 +++++++++++++++
 mg-schema-create          |   20 ++++
 mg-schema-test-database   |    6 +-
 mg-schema-update          |  255 +++++++++++++++++++++++++++++++++++++++++++++
 schema/README.updates     |  179 +++++++++++++++++++++++++++++++
 schema/schema-updates.sql |    6 ++
 6 files changed, 547 insertions(+), 1 deletion(-)
 create mode 100755 mg-schema-update
 create mode 100644 schema/README.updates
 create mode 100644 schema/schema-updates.sql

diff --git a/Osstest/Executive.pm b/Osstest/Executive.pm
index fcef83f..e1fbe3b 100644
--- a/Osstest/Executive.pm
+++ b/Osstest/Executive.pm
@@ -56,6 +56,7 @@ BEGIN {
                       resource_check_allocated resource_shared_mark_ready
                       duration_estimator
                       db_pg_dsn opendb opendb_state
+                      db_schema_updates_applied db_schema_updates_intree
                       );
     %EXPORT_TAGS = ( colours => [qw($green $red $yellow $purple $blue)] );
 
@@ -128,6 +129,87 @@ sub grabrepolock_reexec {
     }
 }
 
+sub db_schema_updates_applied (;$) {
+    my ($cond) = @_;
+    my $r;
+    $cond //= '1=1';
+    eval {
+	local $dbh_tests->{PrintError} = 0;
+	$r = $dbh_tests->selectall_arrayref(<<END);
+	    SELECT updatename, applytime
+              FROM schema_updates WHERE $cond
+END
+    };
+    if ($@) {
+	die unless
+	    $dbh_tests->err()==7 && # DBD::Pg(3pm)
+	    $dbh_tests->state() eq '42P01';
+	# http://www.postgresql.org/docs/current/static/errcodes-appendix.html
+	$r = [ ];
+    }
+    my @r;
+    foreach (@$r) {
+	push @r, { Name => $_->[0], Applied => $_->[1] };
+    }
+    return \@r;
+}
+
+sub db_schema_updates_intree (;$) {
+    my ($incommit) = @_;
+    # ->[]{Name}
+    # ->[]{Seq}
+    # ->[]{State}
+
+    my @results;
+
+    my @files;
+    if (!$incommit) {
+	@files = <schema/*.sql>;
+    } else {
+	local $/ = "\0";
+	open GLF, "-|", qw(git ls-tree -z), $incommit, "schema/" or die $!;
+	while (<GLF>) {
+	    chomp;
+	    next unless s/^\d+ blob \w+\t//;
+	    push @files, $_;
+	}
+	$!=0; $?=0; close GLF or die "$! $? ($incommit)";
+    }
+
+  FILE: foreach my $f (@files) {
+        $f =~ m/\.sql$/ or next;
+	$f =~ m#/([a-z][0-9a-z-]+)\.sql$# or die "badly named .sql file $f\n";
+	my $name = $1;
+	next if $name eq 'initial';
+	if ($incommit) {
+	    open SQLF, "-|", qw(git cat-file blob), "$incommit:$f" or die $!;
+	} else {
+	    open SQLF, "<", $f or die "$f $!";
+	}
+	while (<SQLF>) {
+	    chomp;
+	    my $origl = $_;
+	    next unless s/^\s*--\s*##OSSTEST##\s+//;
+	    m/^0*([1-9]\d*)\s+(Harmless|Preparatory|Unfinished|Ready|Needed)\b/
+		or die "$origl ?";
+	    push @results, {
+		Name => $name,
+		Seq => $1+0,
+		State => $2,
+	    };
+	    next FILE;
+	}
+	$!=0; $?=0; close SQLF; die "$f \`$name' no token ($! $?)";
+    }
+
+    @results = sort {
+	$a->{Seq} <=> $b->{Seq} ||
+	    die "$a->{Name} $a->{Seq} == $b->{Name} $b->{Seq}"
+    } @results;
+
+    return \@results;
+}
+
 #---------- database access ----------#
 
 sub opendb_state () {
diff --git a/mg-schema-create b/mg-schema-create
index 54f1c76..1ee007b 100755
--- a/mg-schema-create
+++ b/mg-schema-create
@@ -30,6 +30,9 @@
 # Options:
 #
 #  -q                            don't print progress messages
+#  --no-updates                  apply no schema updates
+#  --stop-before --stop-after    only apply some schema updates -
+#                                 see mg-schema-update
 
 set -e
 set -o posix
@@ -38,6 +41,8 @@ set -o pipefail
 progress () { printf "%s\n" "$*"; }
 progress=progress
 quietopt=''
+do_updates=true
+updates=()
 
 while [ $# != 0 ]; do
     arg=$1; shift
@@ -46,16 +51,31 @@ while [ $# != 0 ]; do
         progress=:
         quietopt=-q
         ;;
+    --stop-before|--stop-after)
+    	updates+=("$arg" "$1"); shift
+	;;
+    --stop-before=*|--stop-after=*)
+    	updates+=("$arg"); shift
+	;;
+    --no-updates)
+    	do_updates=false
+	;;
     *)
         echo >&2 "bad usage ($arg)"; exit 127
         ;;
     esac
 done
 
+export OSSTEST_DB_USEREAL_IGNORETEST='.*'
+
 . ./cri-getconfig
 
 $progress "Populating database..."
 
 $(get_psql_cmd) $quietopt -f schema/initial.sql
 
+if $do_updates; then
+    ./mg-schema-update $quietopt apply-all "${updates[@]}"
+fi
+
 $progress "Database set up."
diff --git a/mg-schema-test-database b/mg-schema-test-database
index 3616c4d..5c6a935 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -344,6 +344,8 @@ END
 	# Keep a copy as it came from dump, for comparison
 	cp $t.schema $t.schema.orig
 
+	wantupdates=$(./mg-schema-update list-applied)
+
 	# http://www.postgresql.org/message-id/26790.1306355327@sss.pgh.pa.us
 	perl -i~ -pe '
 		s/^/--/ if
@@ -391,7 +393,9 @@ END
 	psql_do <<END
 		CREATE DATABASE $dbname;
 END
-	withtest ./mg-schema-create -q
+	withtest ./mg-schema-create -q --no-updates
+
+	withtest ./mg-schema-update -q apply $wantupdates
 
 	printf ".\n"
 
diff --git a/mg-schema-update b/mg-schema-update
new file mode 100755
index 0000000..2b472fc
--- /dev/null
+++ b/mg-schema-update
@@ -0,0 +1,255 @@
+#!/usr/bin/perl -w
+
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 2009-2015 Citrix Inc.
+# 
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+# 
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+# Usages:
+#
+#  ./mg-schema-update [<options>] apply [<updatename>...]
+#  ./mg-schema-update [<options>] show
+#  ./mg-schema-update [<options>] apply-all
+#
+# Usual rune for applying updates:
+#
+#  ./mg-schema-update -o <oldest-running-git-ref-spec> apply-all
+#
+# Options:
+#
+#  -o<git-ref-spec> --oldest=<git-ref-spec>
+#     Specify the oldest version of osstest that is currently running
+#     anywhere against this DB.  Used to determine compatibility of
+#     updates.
+#
+#  -f
+#     Force.  May be repeated for greater effect:
+#
+#     Force installation of updates which break some old code, even
+#     though -o was not specified so it is not known whether such old
+#     code is still running anywhere.
+#
+#  -ff
+#     Force installation of updates which are known to break running
+#     code (including, perhaps, this very code right here).
+#
+#  -fff
+#     Force an attempt to install an already-installed update.  This
+#     is not likely to work.
+
+
+use strict qw(vars);
+use DBI;
+use Osstest;
+use Osstest::Executive;
+
+use Getopt::Long qw(:config bundling gnu_compat require_order no_ignore_case);
+
+csreadconfig();
+
+our (%state, @state);
+our $there;
+our $force=0;
+our $quiet=0;
+
+sub getstate () {
+    my $record = sub {
+	my ($key,$list) = @_;
+	foreach my $entry (@$list) {
+	    $state{ $entry->{Name} }{ $key } = $entry;
+	}
+    };
+
+    $record->('Db', db_schema_updates_applied());
+    $record->('Here', db_schema_updates_intree());
+    $record->('There', db_schema_updates_intree($there)) if $there;
+
+    foreach my $name (keys %state) {
+	my $out = $state{$name};
+	$out->{Name} = $name;
+	$out->{Sortkey} =
+	    $out->{Here}{Seq} //
+	    $out->{There}{Seq} //
+	    0;
+    }
+
+    foreach my $out (values %state) {
+	my $st = $out->{Here}{State} // 'missing';
+	# ->{Todo} = 0: no; 1: maybe (?old code); 2: always
+	#             -1: already applied
+	if ($out->{Db}{Applied}) {
+	    $out->{Todo} = -1;
+	    $out->{Msg} = "already applied";
+	} elsif ($st =~ m/Harmless|Preparatory/) {
+	    $out->{Todo} = 2;
+	    $out->{Msg} = "right away";
+	} elsif ($st =~ m/Ready|Needed/) {
+	    my $tst = $out->{There}{State} // 'missing';
+	    if (!$there) {
+		$out->{Todo} = 1;
+		$out->{Msg} = "would break any old code";
+	    } elsif ($tst =~ m/Harmless|Preparatory/) {
+		$out->{Todo} = 2;
+		$out->{Msg} = "specified revision can cope";
+	    } else {
+		$out->{Todo} = 0;
+		$out->{Msg} = "specified revision would break";
+	    }
+	} else {
+	    $out->{Todo} = 0;
+	    $out->{Msg} = "not ready";
+	}
+	die unless defined $out->{Todo} && defined $out->{Msg};
+
+	$out->{File} = "schema/$out->{Name}.sql";
+    }
+
+    @state = sort { $a->{Sortkey} <=> $b->{Sortkey} } values %state;
+}
+
+sub cmd_list_applied () {
+    die if @ARGV;
+    getstate();
+    foreach my $v (@state) {
+	next unless $v->{Db}{Applied};
+	print $v->{Name}, "\n" or die $!;
+    }
+}
+
+sub cmd_show () {
+    die if @ARGV;
+
+    getstate();
+
+    printf "%-25s", "Name";
+    printf "    %-9s", "Worktree";
+    printf "  %-11.11s", (sprintf "%8.11s", $there) if $there;
+    printf "  DB update\n";
+
+    foreach my $v (@state) {
+	printf(" %-25s %5s %-5.5s",
+	       $v->{Name},
+	       $v->{Here}{Seq} // '',
+	       $v->{Here}{State} // '-');
+	printf("  %5s %-5.5s",
+	       $v->{There}{Seq} // '',
+	       $v->{There}{State} // '-') if $there;
+	my $app;
+	if ($v->{Db}{Applied}) {
+	    $app = show_abs_time($v->{Db}{Applied});
+	} else {
+	    $app = (qw(No: Maybe: Ready!))[$v->{Todo}];
+	    $app .= " ";
+	    $app .= $v->{Msg};
+	}
+	printf("  %s\n", $app);
+    }
+
+    STDOUT->error and die $!;
+}
+
+sub want_apply ($) {
+    my ($v) = @_;
+    $v->{Todo} >= 2-$force;
+}
+
+sub applyone ($) {
+    my ($v) = @_;
+    die "Will not apply $v->{Name}.sql: $v->{Msg}\n"
+	unless want_apply($v);
+
+    my $fn = $v->{File};
+
+    db_retry($dbh_tests, \@all_lock_tables, sub {
+	print "Applying $fn...\n" unless $quiet;
+	open F, "<", $fn or die "$fn: $!";
+	local $/ = undef;
+	my $sql = <F>;
+	F->error and die $!;
+	close F or die $!;
+
+	$dbh_tests->do(<<END) if $quiet;
+            SET client_min_messages = warning;
+END
+
+	$dbh_tests->do($sql);
+
+	$dbh_tests->do(<<END, {}, $v->{Name}, time);
+	    INSERT INTO schema_updates
+		(updatename, applytime)
+		VALUES (?, ?)
+END
+    });
+
+    print "Applying $fn done.\n" unless $quiet;
+}
+
+sub cmd_apply () {
+    print "No updates applied by calling apply with no update names.\n"
+	unless $quiet;
+    foreach my $name (@ARGV) {
+	getstate();
+	my $ent = $state{$name};
+	die "unknown update \`$name'\n" unless $ent;
+	die "update \`$name' not in this tree\n" unless $ent->{Here}{Seq};
+	applyone($ent);
+    }
+}
+
+sub cmd_apply_all () {
+    my (%stopafter, %stopbefore);
+
+    GetOptions('--stop-after=s' => sub { $stopafter{$_[1]} = 1 },
+	       '--stop-before=s' => sub { $stopbefore{$_[1]} = 1 });
+    die "further arguments to apply-all prohibited\n" if @ARGV;
+
+    getstate();
+    foreach my $v (@state) {
+	next unless $v->{Here}{Seq};
+	my $stop = sub {
+	    my ($map) = @_;
+	    return 0 unless
+		$map->{ $v->{Name} } ||
+		$map->{ $v->{Here}{Seq} };
+	    print "Stopping at $v->{Name} ($v->{Here}{Seq}).\n"
+		unless $quiet;
+	    return 1;
+	};
+
+	last if $stop->(\%stopbefore);
+
+	if (want_apply($v)) {
+	    applyone($v);
+	} else {
+	    print "Skipping $v->{File}: $v->{Msg}\n" unless $quiet;
+	}
+
+	last if $stop->(\%stopafter);
+    }
+
+    print "Appropriate updates applied.\n" unless $quiet;
+}
+
+GetOptions('f|force+' => \$force,
+	   'q+' => \$quiet,
+	   'o|oldest=s' => \$there);
+
+die "need operation\n" unless @ARGV;
+
+my $subcmd= shift @ARGV;
+$subcmd =~ s/-/_/g;
+my $subcmdproc = ${*::}{"cmd_$subcmd"};
+die "unknown subcommand" unless $subcmdproc;
+$subcmdproc->();
diff --git a/schema/README.updates b/schema/README.updates
new file mode 100644
index 0000000..622410c
--- /dev/null
+++ b/schema/README.updates
@@ -0,0 +1,179 @@
+SCHEMA DEFINITION AND SCHEMA UPDATES (PRODUCTION `EXECUTIVE' MODE)
+==================================================================
+
+To generate a new DB, we apply the original schema (in initial.sql)
+and then apply all the updates, in order.
+
+We maintain a table in the DB which records which updates are applied.
+
+
+Schema update snippet format
+----------------------------
+
+Schema update snippets should be called
+   schema/<updatename>.sql
+
+They should contain DDL commands (ALTER TABLE etc.) to make whatever
+changes are needed.
+
+They MUST NOT contain BEGIN or COMMIT.
+
+They must contain a special comment near the top:
+
+  -- ##OSSTEST## <sequence> <status>
+
+<updatename> is a string (/^[a-z][0-9a-z-]+$/) which uniquely identifies
+the update.  It must not be changed because existing installations
+rely on updates having stable names.
+
+<sequence> is a positive integer, which should be unique.  Updates are
+applied in order.
+
+<status> reflects the compatibility of various schema versions.  It is
+a literal string naming one of the statuses shown in `Update orders',
+below.
+
+<status> depends on the nature of the specific database change, and
+the behaviour and capabilities of the other code in the same revision
+of osstest.git.  But, so <status> does not depend on the state of the
+database.  Applying a schema update to a database does not change its
+`status'.
+
+In principle, each update can separately be in applied or not applied
+in any one moment in time (in various databases), and simultaneously
+have different statuses in different relevant versions of osstest.git.
+So overall the possible states of an update in the whole world are the
+cross product of (i) status in each relevant osstest revision, and
+(ii) appliedness (boolean) in each relevant database instance.
+
+
+Update orders
+-------------
+
+There are four reasonable plans for schema changes:
+
+ * Fully intercompatible: both old code and new code are each
+   compatible with both old schema and new schema.  The code and
+   schema updates may be done in any order.
+
+   Such a schema change always has status:
+      Harmless
+
+ * Explicit conditional: first update the code to understand both
+   versions of the schema; then update the schema; then drop the
+   compatibility code.
+
+   Such a schema change always has status:
+      Unfinished (or absent)   in old code
+      Ready                    in intermediate code
+      Needed                   in the final code
+
+ * Code first: the new code works with either old or new schema,
+   but the old code cannot cope with the new schema.
+
+   Such a schema change has status:
+      Unfinished (or absent)   in old code
+      Ready                    in new code
+
+ * Schema first: the new schema works with any code; but the old
+   schema does not work with new code.
+
+   Such a schema change has status:
+      Preparatory              in old code
+      Needed                   in the new code
+
+
+Update order for Populate-then-rely
+-----------------------------------
+
+This is for when we want to record new information and then later rely
+on it.  There are typically two schema changes:
+
+* To add the column(s).  I will call this `add'.  It is a `Schema
+  first' change, in the taxonomy above.
+
+* To add appropriate constraints, to prevent the new information being
+  left blank.  I will call this `constraint'.  This is a `Code first'
+  or `Explicit conditional' change in the taxonomy above.
+
+1. Commit: new schema update `add', status Preparatory.
+
+2. Commit: new schema update `constraint', status Unfinished.
+
+3. Apply: `add'.
+
+4. Optionally commit: code to read new column, but which tolerates
+   both complete absence of the column, and/or it containing NULL
+   (or whatever the DEFAULT value is).
+
+5. Commit: code to populate new column; changing `add' to status
+   Needed and `constraint' to status Ready.
+
+6. Optionally commit: code which read new column, but which tolerates
+   it containing NULL/DEFAULT.  (`add' is already Needed.)
+
+7. If necessary commit: idempotent utility script to populate missing
+   data.  (Alternatively, this can be done with DML statements in the
+   `constraint' schema update .sql file.  This is better if it is
+   possible.)
+
+8. Wait for all executions of old code to finish.  (This obviously
+   implies first getting a push of all the commits mentioned above.)
+
+8. If necessary, execute utility script to populate missing data.
+
+9. Apply: `constraint'.
+
+10. Optionally commit: code which relies on new column, and does not
+   necessarily tolerate NULL/DEFAULT; changing `constraint' to Needed.
+
+
+`Commit' means committing somewhere public and probably pushing to
+osstest.git#pretest, but not necessarily getting a push.  (It
+necessarily precedes any formal testing of the relevant changes on a
+production instance.)
+
+`Apply' (and `execute utility script') should only be done using a
+properly acked version of osstest.git.  If verifying the sanity of the
+schema change is nontrivial then ad-hoc tests may need to have been
+run with a testing instance of the database.  Using only a pushed
+production version is a good idea to avoid the possibility that the
+production database might contain changes which are not evident in
+published code (or worse, which are different in future versions).
+
+Subject to those conditions, `Apply' means an administrator running
+./mg-schema-update as osstest; if `wait for executions of old code to
+finish is needed', this will usually involve passing an appropriate
+`-o' option.
+
+
+Statuses and rules for push and db update
+-----------------------------------------
+
+  Harmless
+  Preparatory
+     No restrictions
+
+  Unfinished
+  (sql fragment entirely missing is equivalent to Unfinished)
+     Schema update: prevented
+     Code push: unrestricted
+
+  Ready
+     Schema update: need all live code to be Preparatory/Ready/Needed
+     Code push: unrestricted
+
+  Needed
+     Schema update: need all live code to be Preparatory/Ready/Needed
+     Code push: depends on schema update
+
+
+"Code push: depends on schema update" is not currently implemented.
+However, many (most?) such changes would cause the push gate itself to
+fail.
+
+"Need all live code to be ..." means to look for the status of this
+schema update in other running versions of osstest.  An attempt at
+this is provided in the form of the `-o' option to mg-schema-update.
+It is the administrator's responsibility to select an appropriate
+argument to `-o'.
diff --git a/schema/schema-updates.sql b/schema/schema-updates.sql
new file mode 100644
index 0000000..cd8dc0c
--- /dev/null
+++ b/schema/schema-updates.sql
@@ -0,0 +1,6 @@
+-- ##OSSTEST## 001 Harmless
+
+CREATE TABLE schema_updates (
+    updatename TEXT PRIMARY KEY,
+    applytime integer NOT NULL
+);
-- 
1.7.10.4

Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates

[Ian Campbell]

On Thu, 2015-12-10 at 17:12 +0000, Ian Jackson wrote:
> diff --git a/schema/README.updates b/schema/README.updates
> new file mode 100644
> index 0000000..622410c
> --- /dev/null
> +++ b/schema/README.updates
> @@ -0,0 +1,179 @@
> +SCHEMA DEFINITION AND SCHEMA UPDATES (PRODUCTION `EXECUTIVE' MODE)
> +==================================================================
> +
> +To generate a new DB, we apply the original schema (in initial.sql)
> +and then apply all the updates, in order.
> +
> +We maintain a table in the DB which records which updates are applied.
> +
> +
> +Schema update snippet format
> +----------------------------
> +
> +Schema update snippets should be called
> +   schema/<updatename>.sql
> +
> +They should contain DDL commands (ALTER TABLE etc.) to make whatever
> +changes are needed.
> +
> +They MUST NOT contain BEGIN or COMMIT.
> +
> +They must contain a special comment near the top:
> +
> +  -- ##OSSTEST## <sequence> <status>
> +
> +<updatename> is a string (/^[a-z][0-9a-z-]+$/) which uniquely identifies
> +the update.  It must not be changed because existing installations
> +rely on updates having stable names.
> +
> +<sequence> is a positive integer, which should be unique.  Updates are
> +applied in order.

Don't these also need to be monotonically increasing over time/commits?

i.e. committing (and applying through all the states) sequence #42 and then
later committing #12 would be at best confusing and at worse perhaps
produce different results when recreating the db (which, I think, would run
#12 first).

So maybe the rule needs to be something about being larger than the largest
currently applied patch?


> +
> +<status> reflects the compatibility of various schema versions.  It is
> +a literal string naming one of the statuses shown in `Update orders',
> +below.
> +
> +<status> depends on the nature of the specific database change, and
> +the behaviour and capabilities of the other code in the same revision
> +of osstest.git.  But, so <status> does not depend on the state of the
> +database.  Applying a schema update to a database does not change its
> +`status'.

"But, so ..." ? I think maybe s/But, s/S/ is what you meant? Or maybe there
was a missing thought?

It's not stated outright, but AIUI the <status> of an update changes in a
commit which either adds/edits a schema update, or which adds code which
adds compatibility/requirements for a particular schema update. Is that
right?

> +In principle, each update can separately be in applied or not applied
> +in any one moment in time (in various databases), and simultaneously
> +have different statuses in different relevant versions of osstest.git.
> +So overall the possible states of an update in the whole world are the
> +cross product of (i) status in each relevant osstest revision, and
> +(ii) appliedness (boolean) in each relevant database instance.
> +
> +
> +Update orders
> +-------------
> +
> +There are four reasonable plans for schema changes:
> +
> + * Fully intercompatible: both old code and new code are each
> +   compatible with both old schema and new schema.  The code and
> +   schema updates may be done in any order.
> +
> +   Such a schema change always has status:
> +      Harmless

What happens if some subsequent change (perhaps a long time later) causes
the code to require the changes made by a "Harmless" schema update?

Would that be a bug in that later code for not coping with the old schema,
or a bug in the commit adding it for not updating the header of the schema
update (to "Needed", presumably) or is it not an issue because there is
some point at which a schema update becomes part of the assumed baseline?

> +
> + * Explicit conditional: first update the code to understand both
> +   versions of the schema; then update the schema; then drop the
> +   compatibility code.
> +
> +   Such a schema change always has status:
> +      Unfinished (or absent)   in old code
> +      Ready                    in intermediate code
> +      Needed                   in the final code

So, a plausible sequence of commits to osstest.git might be:

1: Add schema/foo.update with initial status "Unfinished".
2a: Add code to partially implement compat with the new schema:
        no status change
[2b,2c...] more compat or unrelated changes
3: Add final code to completely implement compat with the new schema:
        status changed to "Ready" in that same commit
4a,b,c: Maybe other unrelated changes
5: Remove any piece code which provides support for the old schema:
        status changes to "Needed"
6: Eventually remove remove other compat code.

I've broken down the addition/removal of compat code into stages to
illustrate, that code might really all come/go in a single commit.

(I see now that there is a more comprehensive example further down the doc)

> +
> + * Code first: the new code works with either old or new schema,
> +   but the old code cannot cope with the new schema.
> +
> +   Such a schema change has status:
> +      Unfinished (or absent)   in old code
> +      Ready                    in new code
> +
> + * Schema first: the new schema works with any code; but the old
> +   schema does not work with new code.
> +
> +   Such a schema change has status:
> +      Preparatory              in old code
> +      Needed                   in the new code
> +
> +
> +Update order for Populate-then-rely
> +-----------------------------------
> +
> +This is for when we want to record new information and then later rely
> +on it.  There are typically two schema changes:
> +
> +* To add the column(s).  I will call this `add'.  It is a `Schema
> +  first' change, in the taxonomy above.
> +
> +* To add appropriate constraints, to prevent the new information being
> +  left blank.  I will call this `constraint'.  This is a `Code first'
> +  or `Explicit conditional' change in the taxonomy above.
> +
> +1. Commit: new schema update `add', status Preparatory.
> +
> +2. Commit: new schema update `constraint', status Unfinished.

At this point we want to wait for those commits to pass the push gate,
before we can apply `add', since applying `add' should be done from a
"properly acked version of osstest.git".

This is made pretty clear by the following commentary, for `Apply' but I
wanted to check I'd got the placement of the wait correct.

> +3. Apply: `add'.
> +
> +4. Optionally commit: code to read new column, but which tolerates
> +   both complete absence of the column, and/or it containing NULL
> +   (or whatever the DEFAULT value is).
> +
> +5. Commit: code to populate new column; changing `add' to status
> +   Needed and `constraint' to status Ready.
> +
> +6. Optionally commit: code which read new column, but which tolerates
> +   it containing NULL/DEFAULT.  (`add' is already Needed.)
> +
> +7. If necessary commit: idempotent utility script to populate missing
> +   data.  (Alternatively, this can be done with DML statements in the
> +   `constraint' schema update .sql file.  This is better if it is
> +   possible.)
> +
> +8. Wait for all executions of old code to finish.  (This obviously
> +   implies first getting a push of all the commits mentioned above.)
> +
> +8. If necessary, execute utility script to populate missing data.
> +
> +9. Apply: `constraint'.
> +
> +10. Optionally commit: code which relies on new column, and does not
> +   necessarily tolerate NULL/DEFAULT; changing `constraint' to Needed.
> +
> +
> +`Commit' means committing somewhere public and probably pushing to
> +osstest.git#pretest, but not necessarily getting a push.  (It
> +necessarily precedes any formal testing of the relevant changes on a
> +production instance.)
> +
> +`Apply' (and `execute utility script') should only be done using a
> +properly acked version of osstest.git.  If verifying the sanity of the
> +schema change is nontrivial then ad-hoc tests may need to have been
> +run with a testing instance of the database.  Using only a pushed
> +production version is a good idea to avoid the possibility that the
> +production database might contain changes which are not evident in
> +published code (or worse, which are different in future versions).
> +
> +Subject to those conditions, `Apply' means an administrator running
> +./mg-schema-update as osstest; if `wait for executions of old code to
> +finish is needed', this will usually involve passing an appropriate
> +`-o' option.
> +
> +
> +Statuses and rules for push and db update
> +-----------------------------------------
> +
> +  Harmless
> +  Preparatory
> +     No restrictions
> +
> +  Unfinished
> +  (sql fragment entirely missing is equivalent to Unfinished)
> +     Schema update: prevented

In the case of "entirely missing" "prevented" must really mean "there can't
possibly be anything to do/prevent"?

> +     Code push: unrestricted
> +
> +  Ready
> +     Schema update: need all live code to be Preparatory/Ready/Needed
> +     Code push: unrestricted
> +
> +  Needed
> +     Schema update: need all live code to be Preparatory/Ready/Needed
> +     Code push: depends on schema update
> +
> +
> +"Code push: depends on schema update" is not currently implemented.
> +However, many (most?) such changes would cause the push gate itself to
> +fail.
> +
> +"Need all live code to be ..." means to look for the status of this
> +schema update in other running versions of osstest.  An attempt at
> +this is provided in the form of the `-o' option to mg-schema-update.
> +It is the administrator's responsibility to select an appropriate
> +argument to `-o'.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates"):
> On Thu, 2015-12-10 at 17:12 +0000, Ian Jackson wrote:
> > +<sequence> is a positive integer, which should be unique.  Updates are
> > +applied in order.
> 
> Don't these also need to be monotonically increasing over time/commits?
> 
> i.e. committing (and applying through all the states) sequence #42 and then
> later committing #12 would be at best confusing and at worse perhaps
> produce different results when recreating the db (which, I think, would run
> #12 first).
> 
> So maybe the rule needs to be something about being larger than the largest
> currently applied patch?

It does say that updates are applied in order.  I will add `and
monotonically increasing' after `unique'.

> > +<status> reflects the compatibility of various schema versions.  It is
> > +a literal string naming one of the statuses shown in `Update orders',
> > +below.
> > +
> > +<status> depends on the nature of the specific database change, and
> > +the behaviour and capabilities of the other code in the same revision
> > +of osstest.git.  But, so <status> does not depend on the state of the
> > +database.  Applying a schema update to a database does not change its
> > +`status'.
> 
> "But, so ..." ? I think maybe s/But, s/S/ is what you meant? Or maybe there
> was a missing thought?

I reordered this, and the `so' is spurious.  It should just say `But,
<status> does not depend...'.

> It's not stated outright, but AIUI the <status> of an update changes in a
> commit which either adds/edits a schema update, or which adds code which
> adds compatibility/requirements for a particular schema update. Is that
> right?

Yes.  I think it should be clear from the rest of the discussion and
I'm not sure that adding some more text here would help clarify
things overall.

> > +   Such a schema change always has status:
> > +      Harmless
> 
> What happens if some subsequent change (perhaps a long time later) causes
> the code to require the changes made by a "Harmless" schema update?
> 
> Would that be a bug in that later code for not coping with the old schema,
> or a bug in the commit adding it for not updating the header of the schema
> update (to "Needed", presumably) or is it not an issue because there is
> some point at which a schema update becomes part of the assumed baseline?

If code starts to depend on the schema change, it should be changed to
Needed when the code is changed.  In practice this is not likely to be
a problem because the update will have been applied a long time ago.

> > + * Explicit conditional: first update the code to understand both
> > +   versions of the schema; then update the schema; then drop the
> > +   compatibility code.
> > +
> > +   Such a schema change always has status:
> > +      Unfinished (or absent)   in old code
> > +      Ready                    in intermediate code
> > +      Needed                   in the final code
> 
> So, a plausible sequence of commits to osstest.git might be:
> 
> 1: Add schema/foo.update with initial status "Unfinished".
> 2a: Add code to partially implement compat with the new schema:
>         no status change
> [2b,2c...] more compat or unrelated changes
> 3: Add final code to completely implement compat with the new schema:
>         status changed to "Ready" in that same commit
> 4a,b,c: Maybe other unrelated changes
> 5: Remove any piece code which provides support for the old schema:
>         status changes to "Needed"
> 6: Eventually remove remove other compat code.
> 
> I've broken down the addition/removal of compat code into stages to
> illustrate, that code might really all come/go in a single commit.

Yes.

> > +Update order for Populate-then-rely
> > +-----------------------------------
> > +
> > +This is for when we want to record new information and then later rely
> > +on it.  There are typically two schema changes:
> > +
> > +* To add the column(s).  I will call this `add'.  It is a `Schema
> > +  first' change, in the taxonomy above.
> > +
> > +* To add appropriate constraints, to prevent the new information being
> > +  left blank.  I will call this `constraint'.  This is a `Code first'
> > +  or `Explicit conditional' change in the taxonomy above.
> > +
> > +1. Commit: new schema update `add', status Preparatory.
> > +
> > +2. Commit: new schema update `constraint', status Unfinished.
> 
> At this point we want to wait for those commits to pass the push gate,
> before we can apply `add', since applying `add' should be done from a
> "properly acked version of osstest.git".
> 
> This is made pretty clear by the following commentary, for `Apply' but I
> wanted to check I'd got the placement of the wait correct.

Yes.

> > +Statuses and rules for push and db update
> > +-----------------------------------------
> > +
> > +  Harmless
> > +  Preparatory
> > +     No restrictions
> > +
> > +  Unfinished
> > +  (sql fragment entirely missing is equivalent to Unfinished)
> > +     Schema update: prevented
> 
> In the case of "entirely missing" "prevented" must really mean "there can't
> possibly be anything to do/prevent"?

Well, if you say
   ./mg-schema-update apply this-update-does-not-exist
it will bomb out.

And
   ./mg-schema-update apply-all
will not invent imaginary schema changes out of thin air so that
it can apply them.

So, application of nonexistent schema changes is indeed prevented by
their nonexistence.


Ian.

Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates

[Ian Jackson]

Ian Jackson writes ("Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates"):
> It does say that updates are applied in order.  I will add `and
> monotonically increasing' after `unique'.  [etc etc]

Here is v3 of this patch.  I'll refrain from reposting the whole
series.

Ian.

>From a7fe7187f6237043f8d6aedeb714409b606af8e0 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Thu, 10 Dec 2015 13:26:00 +0000
Subject: [OSSTEST PATCH 5/8] Schema: Support database schema updates

See schema/README.schema, introduced in this patch, for the design.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v3: Fix spurious message from ./mg-schema-updates apply.
    Fix grammar error in README.updates.

v2: Slight increase schema update name length format.
    Docs fixes:
    Change erroneous `three' to `four'.
    Change `state' to `status' throghout.
    Explain scope of <status>.
    Sort out (and renumber) `Update order for Populate-then-rely'.
    Sort out "Statuses" explanations.
    Encourage use of DML update, rather than ad-hoc scripts,
     for populating new columns.
---
 Osstest/Executive.pm      |   82 +++++++++++++++
 mg-schema-create          |   20 ++++
 mg-schema-test-database   |    6 +-
 mg-schema-update          |  255 +++++++++++++++++++++++++++++++++++++++++++++
 schema/README.updates     |  179 +++++++++++++++++++++++++++++++
 schema/schema-updates.sql |    6 ++
 6 files changed, 547 insertions(+), 1 deletion(-)
 create mode 100755 mg-schema-update
 create mode 100644 schema/README.updates
 create mode 100644 schema/schema-updates.sql

diff --git a/Osstest/Executive.pm b/Osstest/Executive.pm
index fcef83f..e1fbe3b 100644
--- a/Osstest/Executive.pm
+++ b/Osstest/Executive.pm
@@ -56,6 +56,7 @@ BEGIN {
                       resource_check_allocated resource_shared_mark_ready
                       duration_estimator
                       db_pg_dsn opendb opendb_state
+                      db_schema_updates_applied db_schema_updates_intree
                       );
     %EXPORT_TAGS = ( colours => [qw($green $red $yellow $purple $blue)] );
 
@@ -128,6 +129,87 @@ sub grabrepolock_reexec {
     }
 }
 
+sub db_schema_updates_applied (;$) {
+    my ($cond) = @_;
+    my $r;
+    $cond //= '1=1';
+    eval {
+	local $dbh_tests->{PrintError} = 0;
+	$r = $dbh_tests->selectall_arrayref(<<END);
+	    SELECT updatename, applytime
+              FROM schema_updates WHERE $cond
+END
+    };
+    if ($@) {
+	die unless
+	    $dbh_tests->err()==7 && # DBD::Pg(3pm)
+	    $dbh_tests->state() eq '42P01';
+	# http://www.postgresql.org/docs/current/static/errcodes-appendix.html
+	$r = [ ];
+    }
+    my @r;
+    foreach (@$r) {
+	push @r, { Name => $_->[0], Applied => $_->[1] };
+    }
+    return \@r;
+}
+
+sub db_schema_updates_intree (;$) {
+    my ($incommit) = @_;
+    # ->[]{Name}
+    # ->[]{Seq}
+    # ->[]{State}
+
+    my @results;
+
+    my @files;
+    if (!$incommit) {
+	@files = <schema/*.sql>;
+    } else {
+	local $/ = "\0";
+	open GLF, "-|", qw(git ls-tree -z), $incommit, "schema/" or die $!;
+	while (<GLF>) {
+	    chomp;
+	    next unless s/^\d+ blob \w+\t//;
+	    push @files, $_;
+	}
+	$!=0; $?=0; close GLF or die "$! $? ($incommit)";
+    }
+
+  FILE: foreach my $f (@files) {
+        $f =~ m/\.sql$/ or next;
+	$f =~ m#/([a-z][0-9a-z-]+)\.sql$# or die "badly named .sql file $f\n";
+	my $name = $1;
+	next if $name eq 'initial';
+	if ($incommit) {
+	    open SQLF, "-|", qw(git cat-file blob), "$incommit:$f" or die $!;
+	} else {
+	    open SQLF, "<", $f or die "$f $!";
+	}
+	while (<SQLF>) {
+	    chomp;
+	    my $origl = $_;
+	    next unless s/^\s*--\s*##OSSTEST##\s+//;
+	    m/^0*([1-9]\d*)\s+(Harmless|Preparatory|Unfinished|Ready|Needed)\b/
+		or die "$origl ?";
+	    push @results, {
+		Name => $name,
+		Seq => $1+0,
+		State => $2,
+	    };
+	    next FILE;
+	}
+	$!=0; $?=0; close SQLF; die "$f \`$name' no token ($! $?)";
+    }
+
+    @results = sort {
+	$a->{Seq} <=> $b->{Seq} ||
+	    die "$a->{Name} $a->{Seq} == $b->{Name} $b->{Seq}"
+    } @results;
+
+    return \@results;
+}
+
 #---------- database access ----------#
 
 sub opendb_state () {
diff --git a/mg-schema-create b/mg-schema-create
index 54f1c76..1ee007b 100755
--- a/mg-schema-create
+++ b/mg-schema-create
@@ -30,6 +30,9 @@
 # Options:
 #
 #  -q                            don't print progress messages
+#  --no-updates                  apply no schema updates
+#  --stop-before --stop-after    only apply some schema updates -
+#                                 see mg-schema-update
 
 set -e
 set -o posix
@@ -38,6 +41,8 @@ set -o pipefail
 progress () { printf "%s\n" "$*"; }
 progress=progress
 quietopt=''
+do_updates=true
+updates=()
 
 while [ $# != 0 ]; do
     arg=$1; shift
@@ -46,16 +51,31 @@ while [ $# != 0 ]; do
         progress=:
         quietopt=-q
         ;;
+    --stop-before|--stop-after)
+    	updates+=("$arg" "$1"); shift
+	;;
+    --stop-before=*|--stop-after=*)
+    	updates+=("$arg"); shift
+	;;
+    --no-updates)
+    	do_updates=false
+	;;
     *)
         echo >&2 "bad usage ($arg)"; exit 127
         ;;
     esac
 done
 
+export OSSTEST_DB_USEREAL_IGNORETEST='.*'
+
 . ./cri-getconfig
 
 $progress "Populating database..."
 
 $(get_psql_cmd) $quietopt -f schema/initial.sql
 
+if $do_updates; then
+    ./mg-schema-update $quietopt apply-all "${updates[@]}"
+fi
+
 $progress "Database set up."
diff --git a/mg-schema-test-database b/mg-schema-test-database
index 3616c4d..5c6a935 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -344,6 +344,8 @@ END
 	# Keep a copy as it came from dump, for comparison
 	cp $t.schema $t.schema.orig
 
+	wantupdates=$(./mg-schema-update list-applied)
+
 	# http://www.postgresql.org/message-id/26790.1306355327@sss.pgh.pa.us
 	perl -i~ -pe '
 		s/^/--/ if
@@ -391,7 +393,9 @@ END
 	psql_do <<END
 		CREATE DATABASE $dbname;
 END
-	withtest ./mg-schema-create -q
+	withtest ./mg-schema-create -q --no-updates
+
+	withtest ./mg-schema-update -q apply $wantupdates
 
 	printf ".\n"
 
diff --git a/mg-schema-update b/mg-schema-update
new file mode 100755
index 0000000..f3ec1d6
--- /dev/null
+++ b/mg-schema-update
@@ -0,0 +1,255 @@
+#!/usr/bin/perl -w
+
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 2009-2015 Citrix Inc.
+# 
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+# 
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+# Usages:
+#
+#  ./mg-schema-update [<options>] apply [<updatename>...]
+#  ./mg-schema-update [<options>] show
+#  ./mg-schema-update [<options>] apply-all
+#
+# Usual rune for applying updates:
+#
+#  ./mg-schema-update -o <oldest-running-git-ref-spec> apply-all
+#
+# Options:
+#
+#  -o<git-ref-spec> --oldest=<git-ref-spec>
+#     Specify the oldest version of osstest that is currently running
+#     anywhere against this DB.  Used to determine compatibility of
+#     updates.
+#
+#  -f
+#     Force.  May be repeated for greater effect:
+#
+#     Force installation of updates which break some old code, even
+#     though -o was not specified so it is not known whether such old
+#     code is still running anywhere.
+#
+#  -ff
+#     Force installation of updates which are known to break running
+#     code (including, perhaps, this very code right here).
+#
+#  -fff
+#     Force an attempt to install an already-installed update.  This
+#     is not likely to work.
+
+
+use strict qw(vars);
+use DBI;
+use Osstest;
+use Osstest::Executive;
+
+use Getopt::Long qw(:config bundling gnu_compat require_order no_ignore_case);
+
+csreadconfig();
+
+our (%state, @state);
+our $there;
+our $force=0;
+our $quiet=0;
+
+sub getstate () {
+    my $record = sub {
+	my ($key,$list) = @_;
+	foreach my $entry (@$list) {
+	    $state{ $entry->{Name} }{ $key } = $entry;
+	}
+    };
+
+    $record->('Db', db_schema_updates_applied());
+    $record->('Here', db_schema_updates_intree());
+    $record->('There', db_schema_updates_intree($there)) if $there;
+
+    foreach my $name (keys %state) {
+	my $out = $state{$name};
+	$out->{Name} = $name;
+	$out->{Sortkey} =
+	    $out->{Here}{Seq} //
+	    $out->{There}{Seq} //
+	    0;
+    }
+
+    foreach my $out (values %state) {
+	my $st = $out->{Here}{State} // 'missing';
+	# ->{Todo} = 0: no; 1: maybe (?old code); 2: always
+	#             -1: already applied
+	if ($out->{Db}{Applied}) {
+	    $out->{Todo} = -1;
+	    $out->{Msg} = "already applied";
+	} elsif ($st =~ m/Harmless|Preparatory/) {
+	    $out->{Todo} = 2;
+	    $out->{Msg} = "right away";
+	} elsif ($st =~ m/Ready|Needed/) {
+	    my $tst = $out->{There}{State} // 'missing';
+	    if (!$there) {
+		$out->{Todo} = 1;
+		$out->{Msg} = "would break any old code";
+	    } elsif ($tst =~ m/Harmless|Preparatory/) {
+		$out->{Todo} = 2;
+		$out->{Msg} = "specified revision can cope";
+	    } else {
+		$out->{Todo} = 0;
+		$out->{Msg} = "specified revision would break";
+	    }
+	} else {
+	    $out->{Todo} = 0;
+	    $out->{Msg} = "not ready";
+	}
+	die unless defined $out->{Todo} && defined $out->{Msg};
+
+	$out->{File} = "schema/$out->{Name}.sql";
+    }
+
+    @state = sort { $a->{Sortkey} <=> $b->{Sortkey} } values %state;
+}
+
+sub cmd_list_applied () {
+    die if @ARGV;
+    getstate();
+    foreach my $v (@state) {
+	next unless $v->{Db}{Applied};
+	print $v->{Name}, "\n" or die $!;
+    }
+}
+
+sub cmd_show () {
+    die if @ARGV;
+
+    getstate();
+
+    printf "%-25s", "Name";
+    printf "    %-9s", "Worktree";
+    printf "  %-11.11s", (sprintf "%8.11s", $there) if $there;
+    printf "  DB update\n";
+
+    foreach my $v (@state) {
+	printf(" %-25s %5s %-5.5s",
+	       $v->{Name},
+	       $v->{Here}{Seq} // '',
+	       $v->{Here}{State} // '-');
+	printf("  %5s %-5.5s",
+	       $v->{There}{Seq} // '',
+	       $v->{There}{State} // '-') if $there;
+	my $app;
+	if ($v->{Db}{Applied}) {
+	    $app = show_abs_time($v->{Db}{Applied});
+	} else {
+	    $app = (qw(No: Maybe: Ready!))[$v->{Todo}];
+	    $app .= " ";
+	    $app .= $v->{Msg};
+	}
+	printf("  %s\n", $app);
+    }
+
+    STDOUT->error and die $!;
+}
+
+sub want_apply ($) {
+    my ($v) = @_;
+    $v->{Todo} >= 2-$force;
+}
+
+sub applyone ($) {
+    my ($v) = @_;
+    die "Will not apply $v->{Name}.sql: $v->{Msg}\n"
+	unless want_apply($v);
+
+    my $fn = $v->{File};
+
+    db_retry($dbh_tests, \@all_lock_tables, sub {
+	print "Applying $fn...\n" unless $quiet;
+	open F, "<", $fn or die "$fn: $!";
+	local $/ = undef;
+	my $sql = <F>;
+	F->error and die $!;
+	close F or die $!;
+
+	$dbh_tests->do(<<END) if $quiet;
+            SET client_min_messages = warning;
+END
+
+	$dbh_tests->do($sql);
+
+	$dbh_tests->do(<<END, {}, $v->{Name}, time);
+	    INSERT INTO schema_updates
+		(updatename, applytime)
+		VALUES (?, ?)
+END
+    });
+
+    print "Applying $fn done.\n" unless $quiet;
+}
+
+sub cmd_apply () {
+    print "No updates applied by calling apply with no update names.\n"
+	if !@ARGV && !$quiet;
+    foreach my $name (@ARGV) {
+	getstate();
+	my $ent = $state{$name};
+	die "unknown update \`$name'\n" unless $ent;
+	die "update \`$name' not in this tree\n" unless $ent->{Here}{Seq};
+	applyone($ent);
+    }
+}
+
+sub cmd_apply_all () {
+    my (%stopafter, %stopbefore);
+
+    GetOptions('--stop-after=s' => sub { $stopafter{$_[1]} = 1 },
+	       '--stop-before=s' => sub { $stopbefore{$_[1]} = 1 });
+    die "further arguments to apply-all prohibited\n" if @ARGV;
+
+    getstate();
+    foreach my $v (@state) {
+	next unless $v->{Here}{Seq};
+	my $stop = sub {
+	    my ($map) = @_;
+	    return 0 unless
+		$map->{ $v->{Name} } ||
+		$map->{ $v->{Here}{Seq} };
+	    print "Stopping at $v->{Name} ($v->{Here}{Seq}).\n"
+		unless $quiet;
+	    return 1;
+	};
+
+	last if $stop->(\%stopbefore);
+
+	if (want_apply($v)) {
+	    applyone($v);
+	} else {
+	    print "Skipping $v->{File}: $v->{Msg}\n" unless $quiet;
+	}
+
+	last if $stop->(\%stopafter);
+    }
+
+    print "Appropriate updates applied.\n" unless $quiet;
+}
+
+GetOptions('f|force+' => \$force,
+	   'q+' => \$quiet,
+	   'o|oldest=s' => \$there);
+
+die "need operation\n" unless @ARGV;
+
+my $subcmd= shift @ARGV;
+$subcmd =~ s/-/_/g;
+my $subcmdproc = ${*::}{"cmd_$subcmd"};
+die "unknown subcommand" unless $subcmdproc;
+$subcmdproc->();
diff --git a/schema/README.updates b/schema/README.updates
new file mode 100644
index 0000000..25bc11a
--- /dev/null
+++ b/schema/README.updates
@@ -0,0 +1,179 @@
+SCHEMA DEFINITION AND SCHEMA UPDATES (PRODUCTION `EXECUTIVE' MODE)
+==================================================================
+
+To generate a new DB, we apply the original schema (in initial.sql)
+and then apply all the updates, in order.
+
+We maintain a table in the DB which records which updates are applied.
+
+
+Schema update snippet format
+----------------------------
+
+Schema update snippets should be called
+   schema/<updatename>.sql
+
+They should contain DDL commands (ALTER TABLE etc.) to make whatever
+changes are needed.
+
+They MUST NOT contain BEGIN or COMMIT.
+
+They must contain a special comment near the top:
+
+  -- ##OSSTEST## <sequence> <status>
+
+<updatename> is a string (/^[a-z][0-9a-z-]+$/) which uniquely identifies
+the update.  It must not be changed because existing installations
+rely on updates having stable names.
+
+<sequence> is a positive integer, which should be unique.  Updates are
+applied in order.
+
+<status> reflects the compatibility of various schema versions.  It is
+a literal string naming one of the statuses shown in `Update orders',
+below.
+
+<status> depends on the nature of the specific database change, and
+the behaviour and capabilities of the other code in the same revision
+of osstest.git.  But, <status> does not depend on the state of the
+database.  Applying a schema update to a database does not change its
+`status'.
+
+In principle, each update can separately be in applied or not applied
+in any one moment in time (in various databases), and simultaneously
+have different statuses in different relevant versions of osstest.git.
+So overall the possible states of an update in the whole world are the
+cross product of (i) status in each relevant osstest revision, and
+(ii) appliedness (boolean) in each relevant database instance.
+
+
+Update orders
+-------------
+
+There are four reasonable plans for schema changes:
+
+ * Fully intercompatible: both old code and new code are each
+   compatible with both old schema and new schema.  The code and
+   schema updates may be done in any order.
+
+   Such a schema change always has status:
+      Harmless
+
+ * Explicit conditional: first update the code to understand both
+   versions of the schema; then update the schema; then drop the
+   compatibility code.
+
+   Such a schema change always has status:
+      Unfinished (or absent)   in old code
+      Ready                    in intermediate code
+      Needed                   in the final code
+
+ * Code first: the new code works with either old or new schema,
+   but the old code cannot cope with the new schema.
+
+   Such a schema change has status:
+      Unfinished (or absent)   in old code
+      Ready                    in new code
+
+ * Schema first: the new schema works with any code; but the old
+   schema does not work with new code.
+
+   Such a schema change has status:
+      Preparatory              in old code
+      Needed                   in the new code
+
+
+Update order for Populate-then-rely
+-----------------------------------
+
+This is for when we want to record new information and then later rely
+on it.  There are typically two schema changes:
+
+* To add the column(s).  I will call this `add'.  It is a `Schema
+  first' change, in the taxonomy above.
+
+* To add appropriate constraints, to prevent the new information being
+  left blank.  I will call this `constraint'.  This is a `Code first'
+  or `Explicit conditional' change in the taxonomy above.
+
+1. Commit: new schema update `add', status Preparatory.
+
+2. Commit: new schema update `constraint', status Unfinished.
+
+3. Apply: `add'.
+
+4. Optionally commit: code to read new column, but which tolerates
+   both complete absence of the column, and/or it containing NULL
+   (or whatever the DEFAULT value is).
+
+5. Commit: code to populate new column; changing `add' to status
+   Needed and `constraint' to status Ready.
+
+6. Optionally commit: code which read new column, but which tolerates
+   it containing NULL/DEFAULT.  (`add' is already Needed.)
+
+7. If necessary commit: idempotent utility script to populate missing
+   data.  (Alternatively, this can be done with DML statements in the
+   `constraint' schema update .sql file.  This is better if it is
+   possible.)
+
+8. Wait for all executions of old code to finish.  (This obviously
+   implies first getting a push of all the commits mentioned above.)
+
+8. If necessary, execute utility script to populate missing data.
+
+9. Apply: `constraint'.
+
+10. Optionally commit: code which relies on new column, and does not
+   necessarily tolerate NULL/DEFAULT; changing `constraint' to Needed.
+
+
+`Commit' means committing somewhere public and probably pushing to
+osstest.git#pretest, but not necessarily getting a push.  (It
+necessarily precedes any formal testing of the relevant changes on a
+production instance.)
+
+`Apply' (and `execute utility script') should only be done using a
+properly acked version of osstest.git.  If verifying the sanity of the
+schema change is nontrivial then ad-hoc tests may need to have been
+run with a testing instance of the database.  Using only a pushed
+production version is a good idea to avoid the possibility that the
+production database might contain changes which are not evident in
+published code (or worse, which are different in future versions).
+
+Subject to those conditions, `Apply' means an administrator running
+./mg-schema-update as osstest; if `wait for executions of old code to
+finish is needed', this will usually involve passing an appropriate
+`-o' option.
+
+
+Statuses and rules for push and db update
+-----------------------------------------
+
+  Harmless
+  Preparatory
+     No restrictions
+
+  Unfinished
+  (sql fragment entirely missing is equivalent to Unfinished)
+     Schema update: prevented
+     Code push: unrestricted
+
+  Ready
+     Schema update: need all live code to be Preparatory/Ready/Needed
+     Code push: unrestricted
+
+  Needed
+     Schema update: need all live code to be Preparatory/Ready/Needed
+     Code push: depends on schema update
+
+
+"Code push: depends on schema update" is not currently implemented.
+However, many (most?) such changes would cause the push gate itself to
+fail.
+
+"Need all live code to be ..." means to look for the status of this
+schema update in other running versions of osstest.  An attempt at
+this is provided in the form of the `-o' option to mg-schema-update.
+It is the administrator's responsibility to select an appropriate
+argument to `-o'.
diff --git a/schema/schema-updates.sql b/schema/schema-updates.sql
new file mode 100644
index 0000000..cd8dc0c
--- /dev/null
+++ b/schema/schema-updates.sql
@@ -0,0 +1,6 @@
+-- ##OSSTEST## 001 Harmless
+
+CREATE TABLE schema_updates (
+    updatename TEXT PRIMARY KEY,
+    applytime integer NOT NULL
+);
-- 
1.7.10.4

Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates

[Ian Campbell]

On Thu, 2015-12-10 at 18:27 +0000, Ian Jackson wrote:
> Ian Jackson writes ("Re: [OSSTEST PATCH 5/8] Schema: Support database
> schema updates"):
> > It does say that updates are applied in order.  I will add `and
> > monotonically increasing' after `unique'.  [etc etc]
> 
> Here is v3 of this patch.  I'll refrain from reposting the whole
> series.
> 
> Ian.
> 
> From a7fe7187f6237043f8d6aedeb714409b606af8e0 Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Thu, 10 Dec 2015 13:26:00 +0000
> Subject: [OSSTEST PATCH 5/8] Schema: Support database schema updates
> 
> See schema/README.schema, introduced in this patch, for the design.

The README looks good now, thanks.

Code comments....

[...]

> diff --git a/mg-schema-create b/mg-schema-create
> index 54f1c76..1ee007b 100755
> --- a/mg-schema-create
> +++ b/mg-schema-create
> [...]
> @@ -46,16 +51,31 @@ while [ $# != 0 ]; do
>          progress=:
>          quietopt=-q
>          ;;
> +    --stop-before|--stop-after)
> +    	updates+=("$arg" "$1"); shift
> +	;;
> +    --stop-before=*|--stop-after=*)
> +    	updates+=("$arg"); shift
> +	;;
> +    --no-updates)
> +    	do_updates=false
> +	;;
>      *)
>          echo >&2 "bad usage ($arg)"; exit 127
>          ;;
>      esac
>  done
>  
> +export OSSTEST_DB_USEREAL_IGNORETEST='.*'

Why remove the safety catch in this context?

... and that seems to be my only comment.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates"):
> On Thu, 2015-12-10 at 18:27 +0000, Ian Jackson wrote:
> > See schema/README.schema, introduced in this patch, for the design.
> 
> The README looks good now, thanks.

Thanks.

> Code comments....

> > diff --git a/mg-schema-create b/mg-schema-create
...
> > +export OSSTEST_DB_USEREAL_IGNORETEST='.*'
> 
> Why remove the safety catch in this context?

The safety catch breaks when run on an empty schema, because there is
no `tasks' table.  Setting this pattern to `.*' triggers a special
case which avoids even trying to see what test DBs exist.

And furthermore the safety catch is not needed because if we are
mistakenly operating on an existing database (test or real) we bomb
out as previously discussed.

I can put this in a comment if you like.

Ian.

Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates

[Ian Campbell]

On Fri, 2015-12-11 at 15:15 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST PATCH 5/8] Schema: Support database
> schema updates"):
> > On Thu, 2015-12-10 at 18:27 +0000, Ian Jackson wrote:
> > > See schema/README.schema, introduced in this patch, for the design.
> > 
> > The README looks good now, thanks.
> 
> Thanks.
> 
> > Code comments....
> 
> > > diff --git a/mg-schema-create b/mg-schema-create
> ...
> > > +export OSSTEST_DB_USEREAL_IGNORETEST='.*'
> > 
> > Why remove the safety catch in this context?
> 
> The safety catch breaks when run on an empty schema, because there is
> no `tasks' table.  Setting this pattern to `.*' triggers a special
> case which avoids even trying to see what test DBs exist.
> 
> And furthermore the safety catch is not needed because if we are
> mistakenly operating on an existing database (test or real) we bomb
> out as previously discussed.

Thanks for the explanation.

> I can put this in a comment if you like.

I suppose a comment next to any such override is a pretty good policy to
have.

With that: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates"):
> On Fri, 2015-12-11 at 15:15 +0000, Ian Jackson wrote:
> > I can put this in a comment if you like.
> 
> I suppose a comment next to any such override is a pretty good policy to
> have.

OK.  See below; I have folded each hunk into the appropriate patch.

> With that: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks,
Ian.

diff --git a/mg-schema-create b/mg-schema-create
index e15ecb9..9bb3040 100755
--- a/mg-schema-create
+++ b/mg-schema-create
@@ -67,9 +67,17 @@ while [ $# != 0 ]; do
 done
 
 export OSSTEST_DB_USEREAL_IGNORETEST='.*'
+# Completely disable this safety catch, because it otherwise breaks
+# when run on an empty schema (since there is no `tasks' table).
+#
+# It is not needed because if we are mistakenly operating on an
+# existing database (test or real) we will bomb out because the first
+# thing in `initial.sql' is to create the already-existing `flights'
+# table ...
 
 . ./cri-getconfig
 
+# ... Unless some update has been applied which removed `flights':
 updates_applied=$(./mg-schema-update list-applied)
 if [ "x$updates_applied" != x ]; then
     ./mg-schema-update show

Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates

[Ian Campbell]

On Fri, 2015-12-11 at 15:34 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST PATCH 5/8] Schema: Support database
> schema updates"):
> > On Fri, 2015-12-11 at 15:15 +0000, Ian Jackson wrote:
> > > I can put this in a comment if you like.
> > 
> > I suppose a comment next to any such override is a pretty good policy
> > to
> > have.
> 
> OK.  See below; I have folded each hunk into the appropriate patch.

Looks good, thanks.

> 
> > With that: Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Thanks,
> Ian.
> 
> diff --git a/mg-schema-create b/mg-schema-create
> index e15ecb9..9bb3040 100755
> --- a/mg-schema-create
> +++ b/mg-schema-create
> @@ -67,9 +67,17 @@ while [ $# != 0 ]; do
>  done
>  
>  export OSSTEST_DB_USEREAL_IGNORETEST='.*'
> +# Completely disable this safety catch, because it otherwise breaks
> +# when run on an empty schema (since there is no `tasks' table).
> +#
> +# It is not needed because if we are mistakenly operating on an
> +# existing database (test or real) we will bomb out because the first
> +# thing in `initial.sql' is to create the already-existing `flights'
> +# table ...
>  
>  . ./cri-getconfig
>  
> +# ... Unless some update has been applied which removed `flights':
>  updates_applied=$(./mg-schema-update list-applied)
>  if [ "x$updates_applied" != x ]; then
>      ./mg-schema-update show

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates

[Ian Campbell]

On Thu, 2015-12-10 at 18:00 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST PATCH 5/8] Schema: Support database
> schema updates"):
> > On Thu, 2015-12-10 at 17:12 +0000, Ian Jackson wrote:
> > > +<sequence> is a positive integer, which should be unique.  Updates
> > > are
> > > +applied in order.
> > 
> > Don't these also need to be monotonically increasing over time/commits?
> > 
> > i.e. committing (and applying through all the states) sequence #42 and
> > then
> > later committing #12 would be at best confusing and at worse perhaps
> > produce different results when recreating the db (which, I think, would
> > run
> > #12 first).
> > 
> > So maybe the rule needs to be something about being larger than the
> > largest
> > currently applied patch?
> 
> It does say that updates are applied in order.

Right I was considering an osstest repo with 01 and 03 in it, which is
applied and all up to date, but then someone adds 02 and applies it
(therefore after 03).

> > It's not stated outright, but AIUI the <status> of an update changes in
> > a
> > commit which either adds/edits a schema update, or which adds code
> > which
> > adds compatibility/requirements for a particular schema update. Is that
> > right?
> 
> Yes.  I think it should be clear from the rest of the discussion and
> I'm not sure that adding some more text here would help clarify
> things overall.

I was just checking that I had groked it correctly, no need to change
anything.

> > > +Statuses and rules for push and db update
> > > +-----------------------------------------
> > > +
> > > +  Harmless
> > > +  Preparatory
> > > +     No restrictions
> > > +
> > > +  Unfinished
> > > +  (sql fragment entirely missing is equivalent to Unfinished)
> > > +     Schema update: prevented
> > 
> > In the case of "entirely missing" "prevented" must really mean "there
> > can't
> > possibly be anything to do/prevent"?
> 
> Well, if you say
>    ./mg-schema-update apply this-update-does-not-exist
> it will bomb out.
> 
> And
>    ./mg-schema-update apply-all
> will not invent imaginary schema changes out of thin air so that
> it can apply them.
> 
> So, application of nonexistent schema changes is indeed prevented by
> their nonexistence.

Right, I was just wanting to check that you didn't have some mad scheme in
mind for knowing about things which don't exist.

It occurs to me now that while "update-foo" might not exist in the
osstest.git where ./mg-schema-update is being run, it might exist in
another one and therefore have been applied and be present in the database.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 5/8] Schema: Support database schema updates"):
> On Thu, 2015-12-10 at 18:00 +0000, Ian Jackson wrote:
> > It does say that updates are applied in order.
> 
> Right I was considering an osstest repo with 01 and 03 in it, which is
> applied and all up to date, but then someone adds 02 and applies it
> (therefore after 03).

In think in principle this is OK if it is OK, IYSWIM.

If not then perhaps we need a dependency system but let's not go there
:-).

> > So, application of nonexistent schema changes is indeed prevented by
> > their nonexistence.
> 
> Right, I was just wanting to check that you didn't have some mad scheme in
> mind for knowing about things which don't exist.

:-).

> It occurs to me now that while "update-foo" might not exist in the
> osstest.git where ./mg-schema-update is being run, it might exist in
> another one and therefore have been applied and be present in the database.

Indeed.  ./mg-schema-update should display such things correctly.  Let
me try it and see...

(osstest)mariner:testing.git> rm schema/schema-updates.sql 
(osstest)mariner:testing.git> OSSTEST_CONFIG=/u/iwj/.xen-osstest/config:local-config.test-database_iwj ./mg-schema-update -oHEAD show
Name                         Worktree       HEAD     DB update
 schema-updates                  -          1 Harml  2015-12-11 15:06:45 Z
 drop-old-resource-log         2 Harml      2 Harml  2015-12-11 15:06:45 Z
(osstest)mariner:testing.git> OSSTEST_CONFIG=/u/iwj/.xen-osstest/config:local-config.test-database_iwj ./mg-schema-update show
Name                         Worktree   DB update
 schema-updates                  -      2015-12-11 15:06:45 Z
 drop-old-resource-log         2 Harml  2015-12-11 15:06:45 Z
(osstest)mariner:testing.git>

... which is as I would hope.

Ian.

[OSSTEST PATCH 6/8] Schema: Check that schema creation and update runs as the right user

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Executive.pm    |    1 +
 README                  |    5 +++++
 mg-schema-create        |    4 +++-
 mg-schema-test-database |    1 +
 mg-schema-update        |   16 ++++++++++++++++
 5 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Osstest/Executive.pm b/Osstest/Executive.pm
index e1fbe3b..f2d29ef 100644
--- a/Osstest/Executive.pm
+++ b/Osstest/Executive.pm
@@ -105,6 +105,7 @@ augmentconfigdefaults(
     QueuePlanUpdateInterval => 300, # seconds
     Repos => "$ENV{'HOME'}/repos",
     BisectionRevisonGraphSize => '600x300',
+    ExecutiveDbOwningRoleRegexp => 'osstest',
 );
 
 augmentconfigdefaults(
diff --git a/README b/README
index 5740ac0..0a346dc 100644
--- a/README
+++ b/README
@@ -571,6 +571,11 @@ ExecutiveDbname_<DB>
    PostgreSQL dbname string for the database <DB>.  Default is to use
    ExecutiveDbnamePat.
 
+ExecutiveDbOwningRoleRegexp
+   Regexp which is supposed to match the database user used for schema
+   changes - because, that role will end up owning the database objects.
+   Defaults to `osstest'.
+
 Adhoc/Custom Flights
 ====================
 
diff --git a/mg-schema-create b/mg-schema-create
index 1ee007b..df5e215 100755
--- a/mg-schema-create
+++ b/mg-schema-create
@@ -25,7 +25,7 @@
 #  do CREATE DATABASE.)
 #
 #  When setting up a production database, mg-schema-create should
-#  be run *AS THE ROLE USER* who is to own all the resources.
+#  be run as the role user who is to own all the resources.
 #
 # Options:
 #
@@ -70,6 +70,8 @@ export OSSTEST_DB_USEREAL_IGNORETEST='.*'
 
 . ./cri-getconfig
 
+./mg-schema-update $quietopt check-user
+
 $progress "Populating database..."
 
 $(get_psql_cmd) $quietopt -f schema/initial.sql
diff --git a/mg-schema-test-database b/mg-schema-test-database
index 5c6a935..bf82c75 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -335,6 +335,7 @@ OwnerDaemonHost $ctrlhost
 QueueDaemonHost $ctrlhost
 OwnerDaemonPort ${ctrlports%,*}
 QueueDaemonPort ${ctrlports#*,}
+ExecutiveDbOwningRoleRegexp .*
 END
 	mv -f $tcfg.tmp $tcfg
 
diff --git a/mg-schema-update b/mg-schema-update
index 2b472fc..1819b3a 100755
--- a/mg-schema-update
+++ b/mg-schema-update
@@ -22,6 +22,7 @@
 #  ./mg-schema-update [<options>] apply [<updatename>...]
 #  ./mg-schema-update [<options>] show
 #  ./mg-schema-update [<options>] apply-all
+#  ./mg-schema-update [<options>] check-user
 #
 # Usual rune for applying updates:
 #
@@ -119,6 +120,13 @@ sub getstate () {
     @state = sort { $a->{Sortkey} <=> $b->{Sortkey} } values %state;
 }
 
+sub check_user () {
+    my $user = $dbh_tests->{pg_user};
+    my $re = $c{ExecutiveDbOwningRoleRegexp};
+    return if $user =~ m/^$re$/o;
+    die "running as wrong user \`$user', expected to match \`$re'\n";
+}
+
 sub cmd_list_applied () {
     die if @ARGV;
     getstate();
@@ -170,6 +178,8 @@ sub applyone ($) {
     die "Will not apply $v->{Name}.sql: $v->{Msg}\n"
 	unless want_apply($v);
 
+    check_user();
+
     my $fn = $v->{File};
 
     db_retry($dbh_tests, \@all_lock_tables, sub {
@@ -242,6 +252,12 @@ sub cmd_apply_all () {
     print "Appropriate updates applied.\n" unless $quiet;
 }
 
+sub cmd_check_user () {
+    die "too many arguments\n" if @ARGV>1;
+    $c{ExecutiveDbOwningRoleRegexp} = shift @ARGV if @ARGV;
+    check_user();
+}
+
 GetOptions('f|force+' => \$force,
 	   'q+' => \$quiet,
 	   'o|oldest=s' => \$there);
-- 
1.7.10.4

[OSSTEST PATCH 7/8] Schema: drop old resource_log table

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 schema/drop-old-resource-log.sql |    8 ++++++++
 1 file changed, 8 insertions(+)
 create mode 100644 schema/drop-old-resource-log.sql

diff --git a/schema/drop-old-resource-log.sql b/schema/drop-old-resource-log.sql
new file mode 100644
index 0000000..9494e1b
--- /dev/null
+++ b/schema/drop-old-resource-log.sql
@@ -0,0 +1,8 @@
+-- ##OSSTEST## 002 Harmless
+--
+-- This table and associated constraints and indices is from a previous
+-- aborted attempt at resource logging.  Nothing actually reads or
+-- writes it.
+
+DROP TABLE resource_log;
+DROP SEQUENCE resource_log_evid_seq;
-- 
1.7.10.4

[OSSTEST PATCH 8/8] Schema: When creating, check that no updates are applied

[Ian Jackson]

If you try to run mg-schema-create on an existing instance it bombs
out right at the beginning because it tries to create the `flights'
table, which already exists.

But in the future the `flights' table might be removed in an update,
which would remove this safety catch.  Then running the create might
partially succeed, leaving debris a production instance.

Detect this situation by looking for applied schema updates, and
bombing out if there are any.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 mg-schema-create |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/mg-schema-create b/mg-schema-create
index df5e215..e15ecb9 100755
--- a/mg-schema-create
+++ b/mg-schema-create
@@ -70,6 +70,13 @@ export OSSTEST_DB_USEREAL_IGNORETEST='.*'
 
 . ./cri-getconfig
 
+updates_applied=$(./mg-schema-update list-applied)
+if [ "x$updates_applied" != x ]; then
+    ./mg-schema-update show
+    echo >&2 'Database already exists with applied updates!'
+    exit 127
+fi
+
 ./mg-schema-update $quietopt check-user
 
 $progress "Populating database..."
-- 
1.7.10.4