Skip to content

Commit

Permalink
[#23182] YSQL: Fix upgrade test failure when using 2.20.3.1 snapshot
Browse files Browse the repository at this point in the history
Summary:
The following upgrade test that uses a 2.20.3.1 initdb snapshot currently fails:

./yb_build.sh release --java-test 'org.yb.pgsql.TestYsqlUpgrade#upgradeIsIdempotent' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.20.3.1/share/initial_sys_catalog_snapshot'

The error looks like an expected diff found in pg_rewrite table:
```
Expected row: ...... :constraintDeps <> :stmt_location 0 :stmt_len 1006})]
Actual row:   ...... :constraintDeps <> :stmt_location 56 :stmt_len 406})]
```

After debugging, I found that this test runs ysql upgrade twice:
1. First time it runs ysql upgrade triggers running the following scripts sequentially
```
V44__19211__yb_stream_id_in_pg_replication_slots.sql
V45__19128__yb_ash.sql
...
V52__22028__yb_reset_analyze_statistics.sql
```
2. Second time it deletes everything from pg_yb_migration table, and run ysql upgrade again.
This caused to run sequentially
```
V9__10150__yb_extension_role.sql
V10__9178__yb_is_local_table_function.sql
...
V52__22028__yb_reset_analyze_statistics.sql
```
The script that caused the test failure is `V33__14209__yb_terminated_queries.sql`.
This is a migration script that predates the 2.20.3.1 initdb snapshot. When the
2.20.3.1 initdb snapshot was built it ran yb_system_views.sql. In
yb_system_views.sql there is "CREATE VIEW yb_terminated_queries ..." but in the
migration script `V33__14209__yb_terminated_queries.sql` it has "CREATE OR REPLACE VIEW
pg_catalog.yb_terminated_queries ...". These two statements differ textually and
therefore their parse results will have different character position, statement
length, etc. If we ran `V33__14209__yb_terminated_queries.sql` when the the initdb
already has the view, the test fails because of parse results changes like
:stmt_len 1006 vs :stmt_len 406.

To fix test failure like this, I changed this test to only rerun the migration
scripts that come after the 2.20.3.1 release by keeping the <baseline> row in
pg_yb_migration. To ensure the default test behavior remains the same,
when running with 2.0.9.0 (the default snapshot), the test still delete every
row from pg_yb_migration table.
Jira: DB-12122

Test Plan:
1. Run the default ysql upgrade test
./yb_build.sh release --java-test 'org.yb.pgsql.TestYsqlUpgrade'

2. Download various releases to get their initdb snapshots to validate the test.

./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.20.3.1/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.0.9.0/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.11.2.0/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.12.12.0/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.14.13.0/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.16.9.0/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.20.2.0/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.14.17.0/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.18.8.0/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.20.4.1/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.18.3.0/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2.21.1.0/share/initial_sys_catalog_snapshot'
./yb_build.sh release --sj --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/yugabyte-2024.1.0.0/share/initial_sys_catalog_snapshot'

Reviewers: yguan

Reviewed By: yguan

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D36523
  • Loading branch information
myang2021 committed Jul 15, 2024
1 parent 4b39933 commit 98d3fed
Showing 1 changed file with 58 additions and 10 deletions.
68 changes: 58 additions & 10 deletions java/yb-pgsql/src/test/java/org/yb/pgsql/TestYsqlUpgrade.java
Original file line number Diff line number Diff line change
Expand Up @@ -1219,27 +1219,75 @@ public void upgradeCheckingIdempotency(boolean useSingleConnection) throws Excep
.get(preSnapshot.catalog.get(MIGRATIONS_TABLE).size() - 1).getInt(0);
final int latestMinorVersion = preSnapshot.catalog.get(MIGRATIONS_TABLE)
.get(preSnapshot.catalog.get(MIGRATIONS_TABLE).size() - 1).getInt(1);
final int totalMigrations = latestMajorVersion + latestMinorVersion;
// totalMigrations does not include <baseline> entry.
final int totalMigrations = preSnapshot.catalog.get(MIGRATIONS_TABLE).size() - 1;
final int baselineMajorVersion = preSnapshot.catalog.get(MIGRATIONS_TABLE)
.get(0).getInt(0);
final int baselineMinorVersion = preSnapshot.catalog.get(MIGRATIONS_TABLE)
.get(0).getInt(1);
final int firstMigrationVersion = preSnapshot.catalog.get(MIGRATIONS_TABLE)
.get(1).getInt(0);
final String baselineName = preSnapshot.catalog.get(MIGRATIONS_TABLE)
.get(0).getString(2);
assertEquals(baselineName, "<baseline>");

// Make sure the latest version is at least as big as the last hardcoded one (it will be
// greater if more migrations were introduced after YSQL upgrade is released).
assertTrue(latestMajorVersion >= lastHardcodedMigrationVersion);
preSnapshot.catalog.remove(MIGRATIONS_TABLE);
preSnapshot.catalog.remove(CATALOG_VERSION_TABLE);

executeSystemTableDml(stmt, "DELETE FROM " + MIGRATIONS_TABLE);
// If we started the test from an initdb snapshot that did not have
// pg_yb_migration table (release 2.0.9.0), we have <baseline> version
// as 0.0. If we started the test from an initdb snapshot that has a
// valid <baseline>, keep the baseline so we do not re-apply the migration
// scripts prior to that <baseline>. This is to let the following
// check between preSnapshot and postSnapshot to pass: initdb runs
// yb_system_views.sql, which has "CREATE VIEW yb_terminated_queries"
// while the migration script V33__14209__yb_terminated_queries.sql
// has "CREATE OR REPLACE VIEW pg_catalog.yb_terminated_queries".
// These two statements differ textually and therefore their parse
// results will have different character position, statement length, etc.
// If we ran V33__14209__yb_terminated_queries.sql when the view already
// exists in preSnapshot (the initdb snapshot), then it would differ
// from postSnapshot (migration snapshot), causing the test to fail.
if (baselineMajorVersion == 0) {
assertEquals(baselineMinorVersion, 0);
assertEquals(totalMigrations, latestMajorVersion + latestMinorVersion);
// This keeps the old test behavior when we upgrade from 2.0.9.0.
executeSystemTableDml(stmt, "DELETE FROM " + MIGRATIONS_TABLE);
} else {
// This allows only running the migrations that come after the initdb
// snapshot. For example, if the initdb snapshot is 43.1 (for release
// 2.20.3.1), we will run migrations V44, V45, ..., etc.
executeSystemTableDml(stmt, "DELETE FROM " + MIGRATIONS_TABLE +
" WHERE name != '<baseline>'");
}
runMigrations(useSingleConnection);

SysCatalogSnapshot postSnapshot = takeSysCatalogSnapshot(stmt);
List<Row> appliedMigrations = postSnapshot.catalog.get(MIGRATIONS_TABLE);
assertEquals("Expected an entry for the last hardcoded migration"
+ " and each migration past that!",
totalMigrations - lastHardcodedMigrationVersion + 1,
appliedMigrations.size());
assertEquals(
lastHardcodedMigrationVersion,
appliedMigrations
.get(0).getInt(0).intValue());
if (baselineMajorVersion == 0) {
assertEquals("Expected an entry for the last hardcoded migration"
+ " and each migration past that!",
totalMigrations - lastHardcodedMigrationVersion + 1,
appliedMigrations.size());
assertEquals(
lastHardcodedMigrationVersion,
appliedMigrations
.get(0).getInt(0).intValue());
} else {
assertEquals("Expected an entry for the <baseline> migration"
+ " and each migration past that!",
totalMigrations + 1,
appliedMigrations.size());
// Since we kept the first <baseline> row at index 0, the first applied
// migration starts from index 1.
assertEquals(
firstMigrationVersion,
appliedMigrations
.get(1).getInt(0).intValue());
}
assertEquals(
latestMajorVersion,
appliedMigrations
Expand Down

0 comments on commit 98d3fed

Please sign in to comment.