Fix the issue where upgrading from PostgreSQL to IvorySQL fails.#1157
Merged
gaoxueyu merged 1 commit intoIvorySQL:IVORY_REL_5_STABLEfrom Jan 29, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR replaces version-based feature detection with runtime column existence probes in pg_dump for IvorySQL compatibility, removes database mode validation in pg_upgrade, and adds short option support for Oracle mode configuration in pg_upgrade. 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/bin/pg_dump/pg_dump.c`:
- Around line 9353-9377: The runtime probe for attisinvisible can return a false
positive if the pg_attribute row is marked dropped; update the ExecuteSqlQuery
used to detect attisinvisible (res_invisible) to include "AND NOT attisdropped"
in its WHERE clause so the check only considers non-dropped attributes, leaving
the surrounding logic that sets attisinvisible_exists and appends to q
(appendPQExpBufferStr) unchanged.
- Around line 7220-7244: The runtime probe for the relhasrowid column can return
a false positive if the attribute exists but is marked dropped; update the
pg_attribute query used in ExecuteSqlQuery (res_rowid) to exclude dropped
attributes by adding a condition like "AND NOT attisdropped" (or "AND
attisdropped = false") so relhasrowid_exists only becomes true for live columns,
leaving the rest of the logic using query/appendPQExpBufferStr and
res_rowid/PQclear unchanged.
🧹 Nitpick comments (1)
src/bin/pg_upgrade/controldata.c (1)
627-629: Consider a non-fatal warning when database modes differ.
Now that the hard check is removed (Line 627, Line 770), a warning would help users notice a mode mismatch without blocking the upgrade.💡 Suggested warning (non-fatal)
-// if (oldctrl->database_mode_is_oracle != newctrl->database_mode_is_oracle) -// pg_fatal("old and new pg_controldata database mode do not match\n"); +if (oldctrl->database_mode_is_oracle != newctrl->database_mode_is_oracle) + pg_log(PG_WARNING, + "old and new pg_controldata database modes differ; proceeding for compatibility");Also applies to: 770-772
Comment on lines
+7220
to
+7244
| /* | ||
| * Check if relhasrowid column exists. This column is IvorySQL-specific | ||
| * for Oracle ROWID compatibility, and doesn't exist in native PostgreSQL. | ||
| * We need to check at runtime rather than relying on remoteVersion because | ||
| * when upgrading from PostgreSQL to IvorySQL, the source database is | ||
| * PostgreSQL which may have the same version number but lacks this column. | ||
| */ | ||
| { | ||
| PGresult *res_rowid; | ||
| bool relhasrowid_exists = false; | ||
|
|
||
| res_rowid = ExecuteSqlQuery(fout, | ||
| "SELECT 1 FROM pg_catalog.pg_attribute " | ||
| "WHERE attrelid = 'pg_class'::regclass " | ||
| "AND attname = 'relhasrowid' " | ||
| "AND attnum > 0 LIMIT 1", | ||
| PGRES_TUPLES_OK); | ||
| relhasrowid_exists = (PQntuples(res_rowid) > 0); | ||
| PQclear(res_rowid); | ||
|
|
||
| if (relhasrowid_exists) | ||
| appendPQExpBufferStr(query, "c.relhasrowid, "); | ||
| else | ||
| appendPQExpBufferStr(query, "false AS relhasrowid, "); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid false positives if the catalog column was dropped.
If relhasrowid is later removed (left as attisdropped), this probe still returns a row and the subsequent SELECT c.relhasrowid will fail. Filter out dropped attributes.
🩹 Proposed fix
- "AND attnum > 0 LIMIT 1",
+ "AND attnum > 0 AND NOT attisdropped LIMIT 1",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /* | |
| * Check if relhasrowid column exists. This column is IvorySQL-specific | |
| * for Oracle ROWID compatibility, and doesn't exist in native PostgreSQL. | |
| * We need to check at runtime rather than relying on remoteVersion because | |
| * when upgrading from PostgreSQL to IvorySQL, the source database is | |
| * PostgreSQL which may have the same version number but lacks this column. | |
| */ | |
| { | |
| PGresult *res_rowid; | |
| bool relhasrowid_exists = false; | |
| res_rowid = ExecuteSqlQuery(fout, | |
| "SELECT 1 FROM pg_catalog.pg_attribute " | |
| "WHERE attrelid = 'pg_class'::regclass " | |
| "AND attname = 'relhasrowid' " | |
| "AND attnum > 0 LIMIT 1", | |
| PGRES_TUPLES_OK); | |
| relhasrowid_exists = (PQntuples(res_rowid) > 0); | |
| PQclear(res_rowid); | |
| if (relhasrowid_exists) | |
| appendPQExpBufferStr(query, "c.relhasrowid, "); | |
| else | |
| appendPQExpBufferStr(query, "false AS relhasrowid, "); | |
| } | |
| /* | |
| * Check if relhasrowid column exists. This column is IvorySQL-specific | |
| * for Oracle ROWID compatibility, and doesn't exist in native PostgreSQL. | |
| * We need to check at runtime rather than relying on remoteVersion because | |
| * when upgrading from PostgreSQL to IvorySQL, the source database is | |
| * PostgreSQL which may have the same version number but lacks this column. | |
| */ | |
| { | |
| PGresult *res_rowid; | |
| bool relhasrowid_exists = false; | |
| res_rowid = ExecuteSqlQuery(fout, | |
| "SELECT 1 FROM pg_catalog.pg_attribute " | |
| "WHERE attrelid = 'pg_class'::regclass " | |
| "AND attname = 'relhasrowid' " | |
| "AND attnum > 0 AND NOT attisdropped LIMIT 1", | |
| PGRES_TUPLES_OK); | |
| relhasrowid_exists = (PQntuples(res_rowid) > 0); | |
| PQclear(res_rowid); | |
| if (relhasrowid_exists) | |
| appendPQExpBufferStr(query, "c.relhasrowid, "); | |
| else | |
| appendPQExpBufferStr(query, "false AS relhasrowid, "); | |
| } |
🤖 Prompt for AI Agents
In `@src/bin/pg_dump/pg_dump.c` around lines 7220 - 7244, The runtime probe for
the relhasrowid column can return a false positive if the attribute exists but
is marked dropped; update the pg_attribute query used in ExecuteSqlQuery
(res_rowid) to exclude dropped attributes by adding a condition like "AND NOT
attisdropped" (or "AND attisdropped = false") so relhasrowid_exists only becomes
true for live columns, leaving the rest of the logic using
query/appendPQExpBufferStr and res_rowid/PQclear unchanged.
Comment on lines
+9353
to
+9377
| /* | ||
| * Check if attisinvisible column exists. This column is IvorySQL-specific | ||
| * for Oracle compatibility, and doesn't exist in native PostgreSQL. | ||
| * We need to check at runtime rather than relying on remoteVersion because | ||
| * when upgrading from PostgreSQL to IvorySQL, the source database is | ||
| * PostgreSQL which may have the same version number but lacks this column. | ||
| */ | ||
| { | ||
| PGresult *res_invisible; | ||
| bool attisinvisible_exists = false; | ||
|
|
||
| res_invisible = ExecuteSqlQuery(fout, | ||
| "SELECT 1 FROM pg_catalog.pg_attribute " | ||
| "WHERE attrelid = 'pg_attribute'::regclass " | ||
| "AND attname = 'attisinvisible' " | ||
| "AND attnum > 0 LIMIT 1", | ||
| PGRES_TUPLES_OK); | ||
| attisinvisible_exists = (PQntuples(res_invisible) > 0); | ||
| PQclear(res_invisible); | ||
|
|
||
| if (attisinvisible_exists) | ||
| appendPQExpBufferStr(q, "a.attisinvisible,\n"); | ||
| else | ||
| appendPQExpBufferStr(q, "'f' AS attisinvisible,\n"); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid false positives if the catalog column was dropped.
Same risk as above for attisinvisible; add NOT attisdropped to the probe.
🩹 Proposed fix
- "AND attnum > 0 LIMIT 1",
+ "AND attnum > 0 AND NOT attisdropped LIMIT 1",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /* | |
| * Check if attisinvisible column exists. This column is IvorySQL-specific | |
| * for Oracle compatibility, and doesn't exist in native PostgreSQL. | |
| * We need to check at runtime rather than relying on remoteVersion because | |
| * when upgrading from PostgreSQL to IvorySQL, the source database is | |
| * PostgreSQL which may have the same version number but lacks this column. | |
| */ | |
| { | |
| PGresult *res_invisible; | |
| bool attisinvisible_exists = false; | |
| res_invisible = ExecuteSqlQuery(fout, | |
| "SELECT 1 FROM pg_catalog.pg_attribute " | |
| "WHERE attrelid = 'pg_attribute'::regclass " | |
| "AND attname = 'attisinvisible' " | |
| "AND attnum > 0 LIMIT 1", | |
| PGRES_TUPLES_OK); | |
| attisinvisible_exists = (PQntuples(res_invisible) > 0); | |
| PQclear(res_invisible); | |
| if (attisinvisible_exists) | |
| appendPQExpBufferStr(q, "a.attisinvisible,\n"); | |
| else | |
| appendPQExpBufferStr(q, "'f' AS attisinvisible,\n"); | |
| } | |
| /* | |
| * Check if attisinvisible column exists. This column is IvorySQL-specific | |
| * for Oracle compatibility, and doesn't exist in native PostgreSQL. | |
| * We need to check at runtime rather than relying on remoteVersion because | |
| * when upgrading from PostgreSQL to IvorySQL, the source database is | |
| * PostgreSQL which may have the same version number but lacks this column. | |
| */ | |
| { | |
| PGresult *res_invisible; | |
| bool attisinvisible_exists = false; | |
| res_invisible = ExecuteSqlQuery(fout, | |
| "SELECT 1 FROM pg_catalog.pg_attribute " | |
| "WHERE attrelid = 'pg_attribute'::regclass " | |
| "AND attname = 'attisinvisible' " | |
| "AND attnum > 0 AND NOT attisdropped LIMIT 1", | |
| PGRES_TUPLES_OK); | |
| attisinvisible_exists = (PQntuples(res_invisible) > 0); | |
| PQclear(res_invisible); | |
| if (attisinvisible_exists) | |
| appendPQExpBufferStr(q, "a.attisinvisible,\n"); | |
| else | |
| appendPQExpBufferStr(q, "'f' AS attisinvisible,\n"); | |
| } |
🤖 Prompt for AI Agents
In `@src/bin/pg_dump/pg_dump.c` around lines 9353 - 9377, The runtime probe for
attisinvisible can return a false positive if the pg_attribute row is marked
dropped; update the ExecuteSqlQuery used to detect attisinvisible
(res_invisible) to include "AND NOT attisdropped" in its WHERE clause so the
check only considers non-dropped attributes, leaving the surrounding logic that
sets attisinvisible_exists and appends to q (appendPQExpBufferStr) unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
-gcommand-line option for database mode configuration.Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.