From 764fc28011272d7b11fc32bd2774fb6167bd1399 Mon Sep 17 00:00:00 2001 From: Minghui Yang Date: Wed, 26 Jun 2024 18:12:09 +0000 Subject: [PATCH] [BACKPORT pg15-cherrypicks][#23033] Allow running YSQL upgrade unit tests with snapshot other than 2.0.9.0 Summary: Original commit: 2ec9224 / D36198 This involves conflict with YB pg15 4638d7d8265b59b15b84271d8afeeeeba277ec23 on TestYsqlUpgrade.java migratingIsEquivalentToReinitdb @Ignore. 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: jason, tfoucher Reviewed By: jason Subscribers: yql Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D36335 --- .../org/yb/minicluster/MiniYBCluster.java | 13 ++- .../java/org/yb/pgsql/TestYsqlUpgrade.java | 94 +++++++++++++------ 2 files changed, 78 insertions(+), 29 deletions(-) diff --git a/java/yb-client/src/test/java/org/yb/minicluster/MiniYBCluster.java b/java/yb-client/src/test/java/org/yb/minicluster/MiniYBCluster.java index f6b4818b94b0..308b223a0395 100644 --- a/java/yb-client/src/test/java/org/yb/minicluster/MiniYBCluster.java +++ b/java/yb-client/src/test/java/org/yb/minicluster/MiniYBCluster.java @@ -585,7 +585,18 @@ private String getYsqlSnapshotFilePath(YsqlSnapshotVersion ver) { private void applyYsqlSnapshot(YsqlSnapshotVersion ver, Map 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); } } diff --git a/java/yb-pgsql/src/test/java/org/yb/pgsql/TestYsqlUpgrade.java b/java/yb-pgsql/src/test/java/org/yb/pgsql/TestYsqlUpgrade.java index c7e3d7ae32b3..c8f943778412 100644 --- a/java/yb-pgsql/src/test/java/org/yb/pgsql/TestYsqlUpgrade.java +++ b/java/yb-pgsql/src/test/java/org/yb/pgsql/TestYsqlUpgrade.java @@ -911,17 +911,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. - *

- * 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}. - */ - @Ignore("YB_TODO: Ignore test till pg15-based snapshot is available (towards the end of project)") - @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" + " oid oid NOT NULL,\n" + " grpname name NOT NULL,\n" + @@ -934,8 +926,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); @@ -947,28 +937,72 @@ 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. + *

+ * 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}. + */ + @Ignore("YB_TODO: Ignore test till pg15-based snapshot is available (towards the end of project)") + @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 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 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; @@ -1077,6 +1111,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 @@ -1554,25 +1589,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, "", null), - reinitdbMigrations.get(0)); // Applied migrations table has a baseline row // followed by rows for all migrations (up to the latest). List 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, "", 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, "", null), appliedMigrations.get(0)); + assertRow(new Row(initialMajorVersion, initialMinorVersion, "", null), + appliedMigrations.get(0)); for (int ver = 1; ver <= totalMigrations; ++ver) { // Rows should be like [1, 0, 'V1__...', ] 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;