Skip to content

Commit

Permalink
[#23033] Allow running YSQL upgrade unit tests with snapshot other th…
Browse files Browse the repository at this point in the history 
…an 2.0.9.0

Summary:
Currently, when running some YSQL upgrade unit tests (e.g.,
org.yb.pgsql.TestYsqlUpgrade#migratingIsEquivalentToReinitdb), we create the
cluster using initial_sys_catalog_snapshot_2.0.9.0_release or
initial_sys_catalog_snapshot_2.0.9.0_debug. When doing investigation of
128-byte PG name (by changing NAMEDATALEN from 64 to 128, some YSQL
upgrade unit tests failed (including `migratingIsEquivalentToReinitdb`). This is
because the snapshot for 2.0.9.0 isn't compatible with the new 128-byte build.
In order to run the test we need a compatible 2.0.9.0 snapshot that is also built
with 128-byte NAMEDATALEN. But our build tools have changed and a simple
git reset back to 2.0.9.0 to do a 128-byte NAMEDATALEN build did not work.

However I was able to make a 2.14 128-byte NAMEDATALEN build which produced a
compatible 2.14 snapshot. But in order to run YSQL upgrade test using 2.14
snapshot instead of 2.0.9.0 snapshot, I had to change the test code. It will be
good to provide a java test argument that can be used to override the default
2.0.9.0 snapshot when running the YSQL upgrade test.

I made a change to allow using --java-test-args to pass the absolute path to the
snapshot directory to override the 2.0.9.0 snapshot. I found several tests had
issue so made some adjustments needed to get them to pass. In particular:
1. Created a helper function `createPgTablegroupTableIfNotExists`: needed for both
`migratingIsEquivalentToReinitdb` and `upgradeCheckingIdempotency`.
2. Some of the number calculations assumed 2.0.9.0 settings and are hard coded,
they need to be adjusted when a different snapshot is used.

Test Plan:
(1) The default test

PASS
./yb_build.sh release --java-test 'org.yb.pgsql.TestYsqlUpgrade'

Note that it uses 2.0.9.0 snapshot to start upgrade

(2) The customized test that uses 2.14.13.0 snapshot to start upgrade

Download yugabyte-2.14.13.0-b13-linux-x86_64.tar to unpack and get a 2.14.13.0
snapshot at /tmp/yugabyte-2.14.13.0/share/initial_sys_catalog_snapshot.

PASS
./yb_build.sh release --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'

(3) Repeated the test steps by making two 128-byte NAMEDATALEN builds with a
change like:
-#define NAMEDATALEN 64
+#define NAMEDATALEN 128

One for master branch, one for 2.14. The latter is used to produce a 128-byte
NAMEDATALEN 2.14 snapshot that is compatible with the 128-byte NAMEDATALEN
master build to test YSQL upgrade.

(3.1) The default test fails as expected because the default snapshot 2.0.9.0
was built with 64-byte NAMEDATALEN and is not compatible with a 128-byte build.

FAIL
./yb_build.sh release --java-test 'org.yb.pgsql.TestYsqlUpgrade'

(3.2) Run the customized test using the 128-byte 2.14 snapshot.
PASS
./yb_build.sh release --java-test 'org.yb.pgsql.TestYsqlUpgrade' --java-test-args '-Dysql_sys_catalog_snapshot_path=/tmp/ysql-sys-catalog-snapshots-128/initial_sys_catalog_snapshot_2.14_release'

Reviewers: yguan

Reviewed By: yguan

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D36198
  • Loading branch information
@myang2021
myang2021 committed Jul 1, 2024
1 parent 4ae68f4 commit 2ec9224
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 28 deletions.
13 changes: 12 additions & 1 deletion java/yb-client/src/test/java/org/yb/minicluster/MiniYBCluster.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,18 @@ private String getYsqlSnapshotFilePath(YsqlSnapshotVersion ver) {
private void applyYsqlSnapshot(YsqlSnapshotVersion ver, Map<String, String> masterFlags) {
// No need to set the flag for LATEST snapshot.
if (ver != YsqlSnapshotVersion.LATEST) {
String snapshotPath = getYsqlSnapshotFilePath(ver);
// If the test argument provides a sys catalog snapshot path, use it.
// Otherwise, use the default snapshot.
String snapshotPath = System.getProperty("ysql_sys_catalog_snapshot_path");
if (snapshotPath == null) {
snapshotPath = getYsqlSnapshotFilePath(ver);
}
File snapshot = new File(snapshotPath);
assertTrue(snapshot != null);
if (!snapshot.isDirectory()) {
LOG.error("directory '{}' does not exist", snapshotPath);
fail();
}
masterFlags.put("initial_sys_catalog_snapshot_path", snapshotPath);
}
}
Expand Down
92 changes: 65 additions & 27 deletions java/yb-pgsql/src/test/java/org/yb/pgsql/TestYsqlUpgrade.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1007,16 +1007,9 @@ public void upgradeIsIdempotentSingleConn() throws Exception {
connCounter.finish();
}

/**
* Verify that applying migrations to and old initdb snapshot transforms pg_catalog to a state
* equivalent to a fresh initdb.
* <p>
* If you see this test failing, please make sure you've added a new YSQL migration as described
* in {@code src/yb/yql/pgwrapper/ysql_migrations/README.md}.
*/
@Test
public void migratingIsEquivalentToReinitdb() throws Exception {
final String createPgTablegroupTable =
/** Ensure that pg_tablegroup table exists. */
private void createPgTablegroupTableIfNotExists() throws Exception {
final String query =
"CREATE TABLE IF NOT EXISTS pg_catalog.pg_tablegroup (\n" +
" grpname name NOT NULL,\n" +
" grpowner oid NOT NULL,\n" +
Expand All @@ -1029,8 +1022,6 @@ public void migratingIsEquivalentToReinitdb() throws Exception {
" table_oid = 8000,\n" +
" row_type_oid = 8002\n" +
")";

final SysCatalogSnapshot preSnapshotCustom, preSnapshotTemplate1;
try (Connection conn = customDbCb.connect();
Statement stmt = conn.createStatement()) {
setSystemRelsModificationGuc(stmt, true);
Expand All @@ -1042,28 +1033,71 @@ public void migratingIsEquivalentToReinitdb() throws Exception {
// However, other system tables like pg_attribute are modified by the creation of this table,
// and so we can't simply remove it from the snapshot. So it is much simpler to create this
// table again than to try to remove all traces of it ever existing.
executeSystemTableDml(stmt, createPgTablegroupTable);
preSnapshotCustom = takeSysCatalogSnapshot(stmt);
executeSystemTableDml(stmt, query);
}
try (Connection conn = template1Cb.connect();
Statement stmt = conn.createStatement()) {
setSystemRelsModificationGuc(stmt, true);
executeSystemTableDml(stmt, createPgTablegroupTable);
executeSystemTableDml(stmt, query);
}
}


/**
* Verify that applying migrations to and old initdb snapshot transforms pg_catalog to a state
* equivalent to a fresh initdb.
* <p>
* If you see this test failing, please make sure you've added a new YSQL migration as described
* in {@code src/yb/yql/pgwrapper/ysql_migrations/README.md}.
*/
@Test
public void migratingIsEquivalentToReinitdb() throws Exception {
final SysCatalogSnapshot preSnapshotCustom, preSnapshotTemplate1;
createPgTablegroupTableIfNotExists();

try (Connection conn = customDbCb.connect();
Statement stmt = conn.createStatement()) {
preSnapshotCustom = takeSysCatalogSnapshot(stmt);
}
try (Connection conn = template1Cb.connect();
Statement stmt = conn.createStatement()) {
preSnapshotTemplate1 = takeSysCatalogSnapshot(stmt);
}

recreateWithYsqlVersion(YsqlSnapshotVersion.EARLIEST);
createDbConnections();

boolean snapshot_has_pg_yb_tablegroup = false;

try (Connection conn = template1Cb.connect();
Statement stmt = conn.createStatement()) {
// Sanity check - no migrations table
assertNoRows(stmt,
"SELECT oid, relname FROM pg_class WHERE relname = '" + MIGRATIONS_TABLE + "'");

List<Row> rows = getRowList(
stmt, "SELECT * FROM pg_class WHERE relname = 'pg_yb_tablegroup'");
snapshot_has_pg_yb_tablegroup = rows.size() > 0;
String snapshotPath = System.getProperty("ysql_sys_catalog_snapshot_path");
// We expect user provided snapshot path contains the version string as part of the
// snapshot directory. As an example, for a 2.14.13.0 release build, it can be
// /tmp/initial_sys_catalog_snapshot_2.14.13.0_release. NOTE: this needs to be an
// absolute path because anything like $HOME and ~ are not expanded.
// We also assume that any initdb created sys catalog snapshot other than 2.0.9.0,
// migrations table is there.
String migrationTableQuery =
"SELECT oid, relname FROM pg_class WHERE relname = '" + MIGRATIONS_TABLE + "'";
if (snapshotPath == null || snapshotPath.contains("2.0.9.0")) {
// Sanity check - no migrations table
assertNoRows(stmt, migrationTableQuery);
} else {
List<Row> migrationTableRows = getRowList(stmt.executeQuery(migrationTableQuery));
assertFalse(migrationTableRows.isEmpty());
}
stmt.execute("CREATE DATABASE " + customDbName);
}

// For example, if we upgrade from initdb-created 2.14 snapshot, there
// is no pg_tablegroup but pg_yb_tablegroup is already there.
if (snapshot_has_pg_yb_tablegroup) {
createPgTablegroupTableIfNotExists();
}
runMigrations(false /* useSingleConnection */);

final SysCatalogSnapshot postSnapshotCustom, postSnapshotTemplate1;
Expand Down Expand Up @@ -1172,6 +1206,7 @@ public void upgradeCheckingIdempotency(boolean useSingleConnection) throws Excep
Statement stmt = conn.createStatement()) {
stmt.execute("CREATE DATABASE " + customDbName);
}
createPgTablegroupTableIfNotExists();
runMigrations(useSingleConnection);

// Ignore pg_yb_catalog_version because we bump current_version disregarding
Expand Down Expand Up @@ -1650,25 +1685,28 @@ private void assertMigrationsWorked(
assertEquals(
"Expected \"fresh\" migrations table to have just one row, got " + reinitdbMigrations,
1, reinitdbMigrations.size());
final int latestMajorVersion = reinitdbMigrations.get(0).getInt(0);
final int latestMinorVersion = reinitdbMigrations.get(0).getInt(1);
final int totalMigrations = latestMajorVersion + latestMinorVersion;
assertRow(new Row(latestMajorVersion, latestMinorVersion, "<baseline>", null),
reinitdbMigrations.get(0));

// Applied migrations table has a baseline row
// followed by rows for all migrations (up to the latest).
List<Row> appliedMigrations = migratedSnapshot.catalog.get(MIGRATIONS_TABLE);
final int initialMajorVersion = appliedMigrations.get(0).getInt(0);
final int initialMinorVersion = appliedMigrations.get(0).getInt(1);
final int latestMajorVersion = reinitdbMigrations.get(0).getInt(0);
final int latestMinorVersion = reinitdbMigrations.get(0).getInt(1);
final int totalMigrations = latestMajorVersion + latestMinorVersion - initialMajorVersion;
assertRow(new Row(latestMajorVersion, latestMinorVersion, "<baseline>", null),
reinitdbMigrations.get(0));
assertEquals(
"Expected applied migrations table to have exactly "
+ (totalMigrations + 1) + " rows, got " + appliedMigrations,
totalMigrations + 1, appliedMigrations.size());
assertRow(new Row(0, 0, "<baseline>", null), appliedMigrations.get(0));
assertRow(new Row(initialMajorVersion, initialMinorVersion, "<baseline>", null),
appliedMigrations.get(0));
for (int ver = 1; ver <= totalMigrations; ++ver) {
// Rows should be like [1, 0, 'V1__...', <recent timestamp in ms>]
Row migrationRow = appliedMigrations.get(ver);
final int majorVersion = Math.min(ver, latestMajorVersion);
final int minorVersion = ver - majorVersion;
final int majorVersion = Math.min(ver, latestMajorVersion) + initialMajorVersion;
final int minorVersion = ver - majorVersion + initialMajorVersion;
assertEquals(majorVersion, migrationRow.getInt(0).intValue());
assertEquals(minorVersion, migrationRow.getInt(1).intValue());
String migrationNamePrefix;
Expand Down

0 comments on commit 2ec9224

Please sign in to comment.