diff --git a/db/README.md b/db/README.md index a3c86157c..dc673cd4e 100644 --- a/db/README.md +++ b/db/README.md @@ -56,11 +56,23 @@ following steps: table statements can be used as is, whereas alter table statements should be written to change any existing tables. - Note that each incremental file MUST be limited to changes to a single - table (otherwise it may hit deadlock when applying on sandbox/production - where it'll be competing against live traffic that may also be locking said - tables but in a different order). It's OK to include these separate Flyway - scripts in a single PR. + If an incremental file changes more than one schema element (table, index, + or sequence), it MAY hit deadlocks when applied on sandbox/production where + it'll be competing against live traffic that may also be locking said + elements but in a different order. The `FlywayDeadlockTest` checks for this + risk for every new incremental file to be merged. Simply put, the test + treats any of the following as a changed element, and raises an error if a + new file has more than one changed elements: + + * A schema element (table, index, or sequence) being altered. + * The table on which an index is created without the `concurrently` + modifier. Please refer to + + the test class's javadoc for more information. + + Any file failing this test should be split up according to the error + message. It's OK to include these separate Flyway scripts in a single PR. + This script should be stored in a new file in the `db/src/main/resources/sql/flyway` folder using the naming pattern diff --git a/db/build.gradle b/db/build.gradle index 1c53b0cf1..22f83229c 100644 --- a/db/build.gradle +++ b/db/build.gradle @@ -168,6 +168,7 @@ dependencies { testRuntimeOnly deps['com.google.flogger:flogger-system-backend'] testImplementation deps['com.google.guava:guava'] testImplementation deps['com.google.truth:truth'] + testImplementation deps['com.google.truth.extensions:truth-java8-extension'] testRuntimeOnly deps['io.github.java-diff-utils:java-diff-utils'] testImplementation deps['org.junit.jupiter:junit-jupiter-api'] testImplementation deps['org.junit.jupiter:junit-jupiter-engine'] diff --git a/db/gradle.lockfile b/db/gradle.lockfile index cc9b26e16..90e253354 100644 --- a/db/gradle.lockfile +++ b/db/gradle.lockfile @@ -53,6 +53,7 @@ com.google.j2objc:j2objc-annotations:1.3=checkstyle,default,deploy_jar,runtimeCl com.google.j2objc:j2objc-annotations:2.8=testCompileClasspath,testRuntimeClasspath com.google.oauth-client:google-oauth-client:1.34.1=default,deploy_jar,runtimeClasspath,testRuntimeClasspath com.google.protobuf:protobuf-java:3.4.0=annotationProcessor,errorprone,testAnnotationProcessor +com.google.truth.extensions:truth-java8-extension:1.1.3=testCompileClasspath,testRuntimeClasspath com.google.truth:truth:1.1.3=testCompileClasspath,testRuntimeClasspath com.googlecode.java-diff-utils:diffutils:1.3.0=annotationProcessor,errorprone,testAnnotationProcessor com.puppycrawl.tools:checkstyle:8.37=checkstyle diff --git a/db/src/test/java/google/registry/sql/flyway/FlywayDeadlockTest.java b/db/src/test/java/google/registry/sql/flyway/FlywayDeadlockTest.java new file mode 100644 index 000000000..725664567 --- /dev/null +++ b/db/src/test/java/google/registry/sql/flyway/FlywayDeadlockTest.java @@ -0,0 +1,237 @@ +// Copyright 2023 The Nomulus Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package google.registry.sql.flyway; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.common.truth.Truth8.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.nio.file.Files.readAllLines; +import static java.util.concurrent.TimeUnit.SECONDS; +import static java.util.regex.Pattern.CASE_INSENSITIVE; +import static java.util.stream.Collectors.joining; + +import com.google.common.base.Splitter; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.flogger.FluentLogger; +import java.io.File; +import java.io.IOException; +import java.nio.file.Path; +import java.util.Map; +import java.util.Optional; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import org.junit.jupiter.api.Test; + +/** + * Checks if new Flyway scripts may cause Database deadlock. + * + *

Flyway deploys each DDL script in one transaction. If a script modifies multiple schema + * elements (table, index, sequence), it MAY hit deadlocks when applied on sandbox/production where + * it'll be competing against live traffic that may also be locking said elements but in a different + * order. This test checks every new script to be merged, counts the elements it locks, and raise an + * error when there are more than one locked elements. + * + *

For deadlock-prevention purpose, we can ignore elements being created or dropped: our + * schema-server compatibility tests ensure that such elements are not accessed by live traffic. + * Therefore, we focus on 'alter' statements. However, 'create index' is a special case: if the + * 'concurrently' modifier is not present, the indexed table is locked. + */ +public class FlywayDeadlockTest { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + // Relative path (from the root of the repository) of the scripts directory. + private static final String FLYWAY_SCRIPTS_DIR = "db/src/main/resources/sql/flyway"; + + // For splitting a shell command into an array of strings. + private static final Splitter SHELL_COMMAND_SPLITTER = Splitter.on(' ').trimResults(); + + // For splitting a multi-statement SQL string into individual statements. + private static final Splitter SQL_TEXT_SPLITTER = + Splitter.on(";").trimResults().omitEmptyStrings(); + + // For splitting the multi-line text containing all new script paths. + private static final Splitter CHANGED_FILENAMES_SPLITTER = + Splitter.on('\n').trimResults().omitEmptyStrings(); + + // Command that returns the git repo's root dir when executed anywhere in the repo. + private static final String GIT_GET_ROOT_DIR_CMD = "git rev-parse --show-toplevel"; + + // Returns the commit hash when the current branch branches of the main branch. + private static final String GIT_FORK_POINT_CMD = "git merge-base origin/master HEAD"; + + // Command template to get changed Flyways scripts, with fork-point to be filled in. This command + // is executed at the root dir of the repo. Any path returned is relative to the root dir. + private static final String GET_CHANGED_SCRIPTS_CMD = + "git diff %s --name-only " + FLYWAY_SCRIPTS_DIR; + + // Map of DDL patterns and the capture group index of the element name in it. + public static final ImmutableMap DDL_PATTERNS = + ImmutableMap.of( + Pattern.compile( + "^\\s*CREATE\\s+(UNIQUE\\s+)?INDEX\\s+" + + "(IF\\s+NOT\\s+EXISTS\\s+)*(public.)?((\\w+)|(\"\\w+\"))\\s+ON\\s+(ONLY\\s+)?" + + "(public.)?((\\w+)|(\"\\w+\"))[^;]+$", + CASE_INSENSITIVE), + 9, + Pattern.compile( + "^\\s*ALTER\\s+INDEX\\s+(IF\\s+EXISTS\\s+)?(public.)?((\\w+)|(\"\\w+\"))[^;]+$", + CASE_INSENSITIVE), + 3, + Pattern.compile( + "^\\s*ALTER\\s+SEQUENCE\\s+(IF\\s+EXISTS\\s+)?(public.)?((\\w+)|(\"\\w+\"))[^;]+$", + CASE_INSENSITIVE), + 3, + Pattern.compile( + "^\\s*ALTER\\s+TABLE\\s+(IF\\s+EXISTS\\s+|ONLY\\s+)*(public.)?((\\w+)|(\"\\w+\"))[^;]+$", + CASE_INSENSITIVE), + 3); + + @Test + public void validateNewFlywayScripts() { + ImmutableList newScriptPaths = findChangedFlywayScripts(); + ImmutableList scriptAndLockedElements = + newScriptPaths.stream() + .filter(path -> parseDdlScript(path).size() > 1) + .map(path -> String.format("%s: %s", path, parseDdlScript(path))) + .collect(toImmutableList()); + + assertWithMessage("Scripts changing more than one schema elements:") + .that(scriptAndLockedElements) + .isEmpty(); + } + + @Test + public void testGetDdlLockedElementName_found() { + ImmutableList ddls = + ImmutableList.of( + "alter table element_name ...", + "ALTER table if EXISTS ONLY \"element_name\" ...", + "alter index element_name \n...", + "Alter sequence public.\"element_name\" ...", + "create index if not exists \"index_name\" on element_name ...", + "create index if not exists \"index_name\" on public.\"element_name\" ...", + "create index if not exists index_name on public.element_name ...", + "create index if not exists \"index_name\" on public.element_name ...", + "create unique index public.index_name on public.\"element_name\" ..."); + ddls.forEach( + ddl -> { + assertThat(getDdlLockedElementName(ddl)).hasValue("element_name"); + }); + } + + @Test + public void testGetDdlLockedElementName_notFound() { + ImmutableList ddls = + ImmutableList.of( + "create table element_name ...;", + "create sequence public.\"element_name\" ...;", + "create index concurrently if not exists index_name on public.element_name ...;", + "create unique index concurrently public.index_name on public.\"element_name\" ...;", + "drop table element_name ...;", + "drop sequence element_name ...;", + "drop INDEX element_name ...;"); + ddls.forEach( + ddl -> { + assertThat(getDdlLockedElementName(ddl)).isEmpty(); + }); + } + + static Optional getDdlLockedElementName(String ddl) { + for (Map.Entry patternEntry : DDL_PATTERNS.entrySet()) { + Matcher matcher = patternEntry.getKey().matcher(ddl); + if (matcher.find()) { + String name = matcher.group(patternEntry.getValue()); + return Optional.of(name.replace("\"", "")); + } + } + return Optional.empty(); + } + + static ImmutableSet parseDdlScript(Path path) { + try { + return SQL_TEXT_SPLITTER + .splitToStream( + readAllLines(path, UTF_8).stream() + .map(line -> line.replaceAll("--.*", "")) + .filter(line -> !line.isBlank()) + .collect(joining(" "))) + .map(FlywayDeadlockTest::getDdlLockedElementName) + .filter(Optional::isPresent) + .map(Optional::get) + .collect(toImmutableSet()); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + static ImmutableList findChangedFlywayScripts() { + try { + String forkPoint; + try { + forkPoint = executeShellCommand(GIT_FORK_POINT_CMD); + } catch (RuntimeException e) { + if (e.getMessage() != null && e.getMessage().contains("not a git repository")) { + logger.atInfo().log("Not in git repo: probably the internal-pr or -ci test."); + return ImmutableList.of(); + } + throw e; + } + String rootDir = executeShellCommand(GIT_GET_ROOT_DIR_CMD); + String changedScriptsCommand = String.format(GET_CHANGED_SCRIPTS_CMD, forkPoint); + ImmutableList changedPaths = + CHANGED_FILENAMES_SPLITTER + .splitToList(executeShellCommand(changedScriptsCommand, Optional.of(rootDir))) + .stream() + .map(pathStr -> rootDir + File.separator + pathStr) + .map(Path::of) + .collect(toImmutableList()); + if (changedPaths.isEmpty()) { + logger.atInfo().log("There are no schema changes."); + } else { + logger.atInfo().log("Found %s new Flyway scripts", changedPaths.size()); + } + return changedPaths; + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + static String executeShellCommand(String command) throws IOException { + return executeShellCommand(command, Optional.empty()); + } + + static String executeShellCommand(String command, Optional workingDir) + throws IOException { + ProcessBuilder processBuilder = + new ProcessBuilder(SHELL_COMMAND_SPLITTER.splitToList(command).toArray(new String[0])); + workingDir.map(File::new).ifPresent(processBuilder::directory); + Process process = processBuilder.start(); + String output = new String(process.getInputStream().readAllBytes(), UTF_8); + String error = new String(process.getErrorStream().readAllBytes(), UTF_8); + try { + process.waitFor(1, SECONDS); + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + } + if (process.exitValue() != 0) { + throw new RuntimeException(error); + } + return output.trim(); + } +}