Merge lp:~stub/launchpad/pending-db-changes into lp:launchpad/db-devel

[Stuart Bishop]

[r=jelmer][ui=none][bug=689642] Fail if we attempt to apply a database script that doesn't terminate with a semicolon.

Per Bug #689642, database patches could be applied locally if they did not terminate with a semicolon, but would fail when applied to a replicated environment. This bug would also apply to trusted.sql and comments.sql.

I fixed this by removing some unnecessary code where we were splitting the patches into statements and failing if we attempt to apply an sql script with no terminating semicolon.

[Jelmer Vernooij (jelmer)]

As discussed on IRC, please clarify the error message to mention that the last non-whitespace character in the file should be a semicolon - in case the last line contains a comment.

review: Approve (code)

=== modified file 'database/schema/upgrade.py'
--- database/schema/upgrade.py	2010-12-07 12:31:55 +0000
+++ database/schema/upgrade.py	2010-12-15 15:09:45 +0000
@@ -496,26 +496,7 @@
 
 
 def apply_patch(con, major, minor, patch, patch_file):
-    log.info("Applying %s" % patch_file)
-    cur = con.cursor()
-    full_sql = open(patch_file).read()
-
-    # Strip comments
-    full_sql = re.sub('(?xms) \/\* .*? \*\/', '', full_sql)
-    full_sql = re.sub('(?xm) ^\s*-- .*? $', '', full_sql)
-
-    # Regular expression to extract a single statement.
-    # A statement may contain semicolons if it is a stored procedure
-    # definition, which requires a disgusting regexp or a parser for
-    # PostgreSQL specific SQL.
-    statement_re = re.compile(
-            r"( (?: [^;$] | \$ (?! \$) | \$\$.*? \$\$)+ )",
-            re.DOTALL | re.MULTILINE | re.VERBOSE
-            )
-    for sql in statement_re.split(full_sql):
-        sql = sql.strip()
-        if sql and sql != ';':
-            cur.execute(sql) # Will die on a bad patch.
+    apply_other(con, patch_file, no_commit=True)
 
     # Ensure the patch updated LaunchpadDatabaseRevision. We could do this
     # automatically and avoid the boilerplate, but then we would lose the
@@ -531,14 +512,21 @@
         con.commit()
 
 
-def apply_other(con, script):
+def apply_other(con, script, no_commit=False):
     log.info("Applying %s" % script)
     cur = con.cursor()
     path = os.path.join(os.path.dirname(__file__), script)
     sql = open(path).read()
+    if not sql.rstrip().endswith(';'):
+        # This is important because patches are concatenated together
+        # into a single script when we apply them to a replicated
+        # environment.
+        log.fatal(
+            "Last non-whitespace character of %s must be a semicolon", script)
+        sys.exit(3)
     cur.execute(sql)
 
-    if options.commit and options.partial:
+    if not no_commit and options.commit and options.partial:
         log.debug("Committing changes")
         con.commit()