-
Notifications
You must be signed in to change notification settings - Fork 809
Upgrade ID column in the database table to BigInteger to support large OID values.#9223 #9520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughBumps schema version to 49, updates the Database model to use BigInteger for Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/pgadmin/model/__init__.py (2)
17-17: Fix typo in comment."migratio" should be "migration".
✏️ Proposed fix
-2) Create an Alembic migratio to ensure that the appropriate changes are +2) Create an Alembic migration to ensure that the appropriate changes are
417-422: Critical type mismatch: Database.server column type doesn't match Server.id.The migration script (018e16dad6aa_.py) only alters the
database.idcolumn to BigInteger, but the model changes theservercolumn to BigInteger while Server.id remains Integer. This creates a foreign key constraint mismatch: a BigInteger column (Database.server) referencing an Integer primary key (Server.id).To fix this:
- Server.id must be upgraded to BigInteger
- All foreign key columns referencing Server.id must also be upgraded to BigInteger: Process.server_id, QueryHistoryModel.sid, SharedServer.osid
- The migration script must be updated to handle all these changes, not just Database.id
🤖 Fix all issues with AI agents
In @web/migrations/versions/018e16dad6aa_.py:
- Around line 20-26: The Database ORM model's id column is still declared as
db.Integer but the migration alters the database to sa.BigInteger; update the
Database model's id declaration (the id = db.Column(...) in the Database model
class) to use db.BigInteger as the column type and keep primary_key=True so the
ORM matches the migrated schema.
In @web/pgadmin/model/__init__.py:
- Line 415: The model's Database.id column still uses db.Integer but the
migration changed it to BigInteger; update the Database class definition where
id = db.Column(...) to use db.BigInteger instead (e.g., id =
db.Column(db.BigInteger, primary_key=True)), and ensure the db/SQLAlchemy import
supports BigInteger so the model and migration types match.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/migrations/versions/018e16dad6aa_.pyweb/pgadmin/model/__init__.py
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: run-python-tests-pg (windows-latest, 15)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (13)
🔇 Additional comments (2)
web/pgadmin/model/__init__.py (1)
36-36: Schema version bump looks correct.The version increment from 48 to 49 aligns with the addition of a new migration.
web/migrations/versions/018e16dad6aa_.py (1)
28-30: Downgrade not implemented - acceptable for pgAdmin upgrades.The downgrade function is intentionally not implemented, which is consistent with pgAdmin's upgrade-only policy.
dc449b9 to
217fc7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (1)
web/pgadmin/model/__init__.py (1)
417-422: Foreign key type mismatch withServer.id.The
servercolumn is nowBigInteger, but it referencesServer.id(line 191) which remainsdb.Integer. This creates a type mismatch in the foreign key relationship.Additionally, per the AI summary, the migration only alters the
idcolumn—not theservercolumn—so the physical schema won't match this model definition.Since
serverstores pgAdmin's internal server ID (not a PostgreSQL OID), it likely doesn't needBigInteger. Consider keeping it asIntegerto matchServer.id:Suggested fix
server = db.Column( - db.BigInteger, + db.Integer, db.ForeignKey(SERVER_ID), nullable=False, primary_key=True )Alternatively, if
BigIntegeris intentional here, thenServer.idand the migration should also be updated for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/migrations/versions/018e16dad6aa_.pyweb/pgadmin/model/__init__.py
🚧 Files skipped from review as they are similar to previous changes (1)
- web/migrations/versions/018e16dad6aa_.py
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-python-tests-pg (windows-latest, 17)
🔇 Additional comments (2)
web/pgadmin/model/__init__.py (2)
36-36: LGTM!Schema version correctly incremented to reflect the model changes, as required by the documented upgrade process.
415-415: LGTM!Appropriate change to
BigIntegerfor storing PostgreSQL OID values, which are unsigned 32-bit integers and can exceed the signedIntegerrange (~2.1 billion).
217fc7e to
ffa1a0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/model/__init__.py (1)
415-422: Critical: Foreign key type mismatch betweenDatabase.serverandServer.id.The
Database.servercolumn is changed toBigInteger(line 418) withForeignKey(SERVER_ID), but it referencesserver.idwhich is defined asInteger(line 191). This type mismatch violates foreign key constraints in most database backends. Additionally, this is inconsistent with other models—QueryHistoryModel.sidandSharedServer.osidboth useIntegerfor theirSERVER_IDforeign keys, makingDatabase.serverthe onlyBigIntegerreference toserver.id.Required: Either upgrade
Server.idtoBigIntegerto support large OID values across all referencing models, or revertDatabase.serverback toIntegerto maintain consistency.
🤖 Fix all issues with AI agents
In @web/migrations/versions/018e16dad6aa_.py:
- Around line 28-33: The migration only alters Database.id to BigInteger but
misses updating the Database.server column; update the upgrade() function
(inside the existing with op.batch_alter_table("database") as batch_op block) to
also call batch_op.alter_column('server', existing_type=sa.Integer(),
type_=sa.BigInteger(), nullable=False) so the database.server column matches the
model change to BigInteger (Database.server).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/migrations/versions/018e16dad6aa_.pyweb/pgadmin/model/__init__.py
🔇 Additional comments (2)
web/migrations/versions/018e16dad6aa_.py (1)
36-38: No-op downgrade is intentional per pgAdmin policy.The downgrade function is a no-op, which aligns with pgAdmin's upgrade-only approach. This is acceptable and properly documented.
web/pgadmin/model/__init__.py (1)
36-36: LGTM: Schema version bumped appropriately.The schema version increment from 48 to 49 follows the documented process and aligns with the model changes and migration being added.
ffa1a0b to
9557803
Compare
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.