Fix the issue where upgrading from PostgreSQL to IvorySQL fails.#1158
Fix the issue where upgrading from PostgreSQL to IvorySQL fails.#1158gaoxueyu merged 1 commit intoIvorySQL:masterfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds runtime catalog capability checks to pg_dump to gracefully handle missing IvorySQL-specific columns (pg_package, relhasrowid, attisinvisible) instead of relying solely on version comparisons. It also reduces strictness of database mode compatibility validation in pg_upgrade and adds support for new command-line options. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bin/pg_upgrade/controldata.c (1)
698-699: Inconsistent reporting for non-mandatory field.This reports "database mode" as missing, but since line 627 was commented out, missing database mode won't trigger this error block unless another mandatory field is also missing. This creates confusing diagnostic output—users would see a report about database mode when it's not actually the cause of the failure.
Either remove this reporting or make the logic consistent with the mandatory check.
🔧 Suggested fix
Remove the reporting if database mode is no longer mandatory:
- if (!got_database_mode_is_oracle) - pg_log(PG_REPORT, " database mode\n"); -Or conditionally report it:
- if (!got_database_mode_is_oracle) - pg_log(PG_REPORT, " database mode\n"); + if (!got_database_mode_is_oracle && pg_cluster_within_oracle_mode) + pg_log(PG_REPORT, " database mode");Also note the inconsistent
\nin the format string compared to otherpg_logcalls in this block.src/bin/pg_upgrade/option.c (2)
199-202: Inconsistent newline in error message.The
pg_fatalcall includes a trailing\n, but otherpg_fatalcalls in this file (e.g., lines 186, 191, 249) don't use trailing newlines.pg_fataltypically handles newlines internally.🔧 Suggested fix
case 'Q': if ((new_cluster.oraport = atoi(optarg)) <= 0) - pg_fatal("invalid new port number\n"); + pg_fatal("invalid new port number"); break;Also apply to line 196:
case 'q': if ((old_cluster.oraport = atoi(optarg)) <= 0) - pg_fatal("invalid old port number\n"); + pg_fatal("invalid old port number"); break;
344-344: Incomplete help text for-goption.The description "upgrade the PG cluster in Oracle mode, please use this option" is unclear—it doesn't explain when or why users should use this option. Users won't understand its purpose from this text.
📝 Suggested improvement
- printf(_(" -g, --using-ora-pg upgrade the PG cluster in Oracle mode, please use this option\n")); + printf(_(" -g, --using-ora-pg allow upgrading from PostgreSQL to IvorySQL\n"));Or provide more context:
printf(_(" -g, --using-ora-pg use when upgrading a PostgreSQL cluster to IvorySQL\n"));
🧹 Nitpick comments (3)
src/bin/pg_upgrade/controldata.c (2)
627-629: Replace commented-out code with proper conditional logic.Using
//to disable code is not maintainable. If database mode is intentionally optional for PostgreSQL→IvorySQL upgrades, refactor to make this explicit rather than leaving dead code.♻️ Suggested refactor
- //!got_database_mode_is_oracle || - (!got_default_char_signedness && - cluster->controldata.cat_ver >= DEFAULT_CHAR_SIGNEDNESS_CAT_VER)) + (!got_default_char_signedness && + cluster->controldata.cat_ver >= DEFAULT_CHAR_SIGNEDNESS_CAT_VER))If
database_mode_is_oracleshould only be required for IvorySQL clusters, consider:/* database_mode_is_oracle is only mandatory for IvorySQL source clusters */ (!got_database_mode_is_oracle && pg_cluster_within_oracle_mode) ||
770-771: Replace commented-out code with conditional logic or remove entirely.Same issue as line 627—using
//to disable a compatibility check is not a clean solution. If mismatched database modes should be allowed for PostgreSQL→IvorySQL upgrades, make this explicit.♻️ Suggested refactor
Option 1 - Remove if truly unnecessary:
-// if (oldctrl->database_mode_is_oracle != newctrl->database_mode_is_oracle) -// pg_fatal("old and new pg_controldata database mode do not match\n");Option 2 - Make conditional on upgrade scenario:
-// if (oldctrl->database_mode_is_oracle != newctrl->database_mode_is_oracle) -// pg_fatal("old and new pg_controldata database mode do not match\n"); + /* + * Database mode mismatch is allowed when upgrading from PostgreSQL + * to IvorySQL (pg_cluster_within_oracle_mode handles this case). + */ + if (!pg_cluster_within_oracle_mode && + oldctrl->database_mode_is_oracle != newctrl->database_mode_is_oracle) + pg_fatal("old and new pg_controldata database mode do not match");src/bin/pg_dump/pg_dump.c (1)
7167-7191: Consider centralizing catalog-capability checks.The relhasrowid probe is good, but it duplicates the pattern used for other IvorySQL-only columns. A small helper (optionally cached in
ArchiveHandle) would keep this consistent and reduce future drift.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.