Add an `--initial-sql` flag to InitDb.pl; allow configuring the `REPLICATION_TYPE` for create_test_db.sh #3197

mwiencek · 2024-03-06T21:08:27Z

This is based on top of #3194

Problem

This resolves two problems:

./admin/InitDb.pl currently always runs admin/sql/InsertDefaultRows.sql on the database, and this cannot be configured (e.g., to specify a different initialization script, or use none at all).
./script/create_test_db.sh currently cannot be used to initialize a mirror test database (i.e., one without foreign keys or triggers).

I'd like to fix these issues in order to improve the test suites that make use of replication packets (and thus require separate master/mirror test databases).

Solution

With this PR, you can set the REPLICATION_TYPE environment variable before running ./script/create_test_db.sh:

REPLICATION_TYPE=1 ./script/create_test_db.sh # master
REPLICATION_TYPE=2 ./script/create_test_db.sh # mirror
REPLICATION_TYPE=3 ./script/create_test_db.sh # standalone

See associated commits for details.

Testing

Just manual invocations of the create_test_db.sh and InitDb.pl commands, plus automated tests.

reosarevok · 2024-03-08T09:10:08Z

t/lib/t/MusicBrainz/Script/Utils.pm

+    is($result, $f1_path, 'scalar file is found by direct path');
+
+    $result = find_mbdump_file('artist', $dir1);
+    is($result, $f1_path, 'scalar file is found in directory');


Is there a reason you prefer lowercase for these labels? I generally start them with uppercase because they look like full sentences to me the way we print them, but obviously this is mega minor anyway :)

I guess because they read more like fragments than full sentences to me. :) And some of them start with undef or variable names which would be weird to capitalize...

I don't have a particular preference though.

reosarevok

LGTMBDNT, I'd like @yvanzo to check it too. I had no idea script/setup_development_db.sh even existed!

This merges the separate implementations of `find_file` contained in admin/MBImport.pl and admin/replication/ImportReplicationChanges into `Script::Utils`, and adds tests for said function. The main difference between the two was that MBImport.pl's implementation would return an array of matching files, rather than the first match. We now use Perl's `wantarray` built-in to distinguish between the two cases.

In case the search paths overlap (e.g., we may find the file via a direct path, and then again via a parent directory).

It works the same, except it doesn't check `mbdump/` subdirectories nor does it support returning a scalar result. `find_mbdump_file` calls it internally.

Support specifying a path prefix in the first `$file` paremeter: e.g., `caa/CreateTables.sql` in the added test case includes a `caa/` path prefix, but will now be found in the search path `/tmp/foo/sql/caa/CreateTables.sql`.

Allows specifying which SQL script to initialize a clean database with. Prior to this commit, admin/sql/InsertDefaultRows.sql was always used (and this could not be disabled!). One current use case of this is that we can initialize test databases with t/sql/initial.sql instead; and in that file, we no longer have to worry about disabling triggers (because the `initial-sql` script is run before triggers are added) or specifying `ON CONFLICT` handlers (which were to avoid conflicts with existing data that was in admin/sql/InsertDefaultRows.sql). The triggers issue is particularly relevant for mirror databases, where certain triggers do not exist. We can now initialize mirror test databases with t/sql/initial.sql, where previously it would error on the `DISABLE TRIGGER a_ins_instrument` line. But removing that line means we *must* run this script before creating triggers, hence the lines I've reordered in create_test_db.sh, and the changes I've made to the Selenium test scripts.

With the `--initial-sql` flag to InitDb.pl, we only have to drop the existing schemas if the database already exists (and otherwise create the pgtap extension).

`ON_ERROR_STOP` must be set in order for psql to return a non-zero exit code on statement failures. This is not much of an issue now that `IF NOT EXISTS` was added, but I thought I'd fix it anyway given that the intent of the code is to capture errors.

I've never once used this, and its purpose seems to have been overtaken by the sample database dumps.

yvanzo

About testing. What has been tested so far? Is there a setup that you would like us to test in particular?

Only minor comments otherwise.

yvanzo · 2024-03-11T16:05:23Z

script/create_selenium_db.sh

+if [ -z "$SIR_DIR" ]; then
+    SIR_DIR="$MB_SERVER_ROOT"/../sir
+fi


Tip for setting a variable to a default value:

Suggested change

if [ -z "$SIR_DIR" ]; then

SIR_DIR="$MB_SERVER_ROOT"/../sir

fi

SIR_DIR="${SIR_DIR:=$MB_SERVER_ROOT/../sir}"

yvanzo · 2024-03-11T16:06:37Z

script/create_selenium_db.sh

+if [ -z "$ARTWORK_INDEXER_DIR" ]; then
+    ARTWORK_INDEXER_DIR="$MB_SERVER_ROOT"/../artwork-indexer
+fi


Same as above:

Suggested change

if [ -z "$ARTWORK_INDEXER_DIR" ]; then

ARTWORK_INDEXER_DIR="$MB_SERVER_ROOT"/../artwork-indexer

fi

ARTWORK_INDEXER_DIR="${ARTWORK_INDEXER_DIR:=$MB_SERVER_ROOT/../artwork-indexer}"

yvanzo · 2024-03-11T16:25:27Z

script/create_selenium_db.sh

@@ -0,0 +1,50 @@
+#!/usr/bin/env bash
+
+set -o errexit


It might be safer to enable all the error detection options then:

nounset detects using unset variables (whether that is due to a coding mistake or a script misuse).

pipefail detects failures of piped commands (even though echo is unlikely to fail here).

Suggested change

set -o errexit

set -o errexit -o nounset -o pipefail

yvanzo · 2024-03-11T16:49:40Z

docker/musicbrainz-tests/run_circleci_tests.sh

@@ -20,7 +20,7 @@ sudo -E -H -u musicbrainz cp docker/musicbrainz-tests/DBDefs.pm lib/

 sv_start_if_down postgresql redis

-sudo -E -H -u musicbrainz carton exec -- ./script/create_test_db.sh
+REPLICATION_TYPE=1 sudo -E -H -u musicbrainz carton exec -- ./script/create_test_db.sh


The reason for this change isn’t obvious from the message for commit 37a8cf3

yvanzo · 2024-03-11T16:49:55Z

t/script/ExportAllTables.t

@@ -117,6 +117,7 @@ test all => sub {
        '-U', $system_db->username,
        $test_db->database;

+    $ENV{REPLICATION_TYPE} = 1; # master


Same as above

mwiencek force-pushed the initdb-initial-sql branch 19 times, most recently from 37842e3 to 54b817a Compare March 8, 2024 03:58

reosarevok reviewed Mar 8, 2024

View reviewed changes

reosarevok requested a review from yvanzo March 8, 2024 09:30

mwiencek added 8 commits March 11, 2024 10:30

find_mbdump_file: Uniquify found files array

6fc9546

In case the search paths overlap (e.g., we may find the file via a direct path, and then again via a parent directory).

Factor out find_files from find_mbdump_file

d0bfebc

It works the same, except it doesn't check `mbdump/` subdirectories nor does it support returning a scalar result. `find_mbdump_file` calls it internally.

find_files: Fix when searching with path prefix

adddab2

Support specifying a path prefix in the first `$file` paremeter: e.g., `caa/CreateTables.sql` in the added test case includes a `caa/` path prefix, but will now be found in the search path `/tmp/foo/sql/caa/CreateTables.sql`.

RunSQLScript: Quote file path for shell

c9b18e0

Simplify script/create_test_db.sh

37a8cf3

With the `--initial-sql` flag to InitDb.pl, we only have to drop the existing schemas if the database already exists (and otherwise create the pgtap extension).

Remove script/setup_development_db.sh

0db1479

I've never once used this, and its purpose seems to have been overtaken by the sample database dumps.

mwiencek force-pushed the initdb-initial-sql branch from 54b817a to 0db1479 Compare March 11, 2024 15:30

yvanzo reviewed Mar 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an `--initial-sql` flag to InitDb.pl; allow configuring the `REPLICATION_TYPE` for create_test_db.sh #3197

Add an `--initial-sql` flag to InitDb.pl; allow configuring the `REPLICATION_TYPE` for create_test_db.sh #3197

mwiencek commented Mar 6, 2024

reosarevok Mar 8, 2024

mwiencek Mar 8, 2024

reosarevok left a comment

yvanzo left a comment

yvanzo Mar 11, 2024

yvanzo Mar 11, 2024

yvanzo Mar 11, 2024

yvanzo Mar 11, 2024

yvanzo Mar 11, 2024

Add an --initial-sql flag to InitDb.pl; allow configuring the REPLICATION_TYPE for create_test_db.sh #3197

Are you sure you want to change the base?

Add an --initial-sql flag to InitDb.pl; allow configuring the REPLICATION_TYPE for create_test_db.sh #3197

Conversation

mwiencek commented Mar 6, 2024

Problem

Solution

Testing

reosarevok Mar 8, 2024

Choose a reason for hiding this comment

mwiencek Mar 8, 2024

Choose a reason for hiding this comment

reosarevok left a comment

Choose a reason for hiding this comment

yvanzo left a comment

Choose a reason for hiding this comment

yvanzo Mar 11, 2024

Choose a reason for hiding this comment

yvanzo Mar 11, 2024

Choose a reason for hiding this comment

yvanzo Mar 11, 2024

Choose a reason for hiding this comment

yvanzo Mar 11, 2024

Choose a reason for hiding this comment

yvanzo Mar 11, 2024

Choose a reason for hiding this comment

Add an `--initial-sql` flag to InitDb.pl; allow configuring the `REPLICATION_TYPE` for create_test_db.sh #3197

Add an `--initial-sql` flag to InitDb.pl; allow configuring the `REPLICATION_TYPE` for create_test_db.sh #3197