Doc: Added missing instructions for schema upgrade from v4 to v5 in 1.7.0 doc - #5437
Doc: Added missing instructions for schema upgrade from v4 to v5 in 1.7.0 doc#5437MonkeyCanCode wants to merge 3 commits into
Conversation
MonkeyCanCode
changed the title
github-project-automation
Bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
github-project-automation
Bot
moved this from Done
to PRs In Progress
in Basic Kanban Board
| DROP TABLE IF EXISTS polaris_schema.idempotency_records; | ||
| ALTER TABLE polaris_schema.events ALTER COLUMN catalog_id DROP NOT NULL; | ||
| UPDATE polaris_schema.events SET catalog_id = NULL WHERE catalog_id = '__realm__'; | ||
| DROP TABLE IF EXISTS polaris_schema.idempotency_records; |
There was a problem hiding this comment.
This DROP TABLE is a duplicate of the one above the ALTER.
There was a problem hiding this comment.
Hello @ayushtkn ,
Yes, this had been updated. Copy/Paste issue. Same responded to the current on-going PR: https://github.com/apache/polaris/pull/5349/changes#r3942654660.
Thanks,
Yong
| CockroachDB, and H2), then restart Polaris: | ||
|
|
||
| ```sql | ||
| DROP INDEX IF EXISTS polaris_schema.idx_idemp_realm_expires; |
There was a problem hiding this comment.
Is DROP INDEX intentional? I think dropping the table drops its index on Postgres, CockroachDB and H2 alike.
this index is created on idempotency_records
polaris/persistence/relational-jdbc/src/main/resources/postgres/schema-v4.sql
Lines 170 to 171 in 8775ef4
which we are dropping below
There was a problem hiding this comment.
FYI I suggested to @MonkeyCanCode dropping the index too, mostly for completeness. I'm fine not dropping it explicitly though, if you think that's better.
There was a problem hiding this comment.
Yes, this is for matching to https://github.com/adutra/polaris/blob/0668f5d5279e1cb367e16cc9991966d01352666b/site/content/in-dev/unreleased/metastores/relational-jdbc.md.
With implicitly drops, I do think it is easier if people want to revert the changes (as now they see the implicit removal of tables and indices). However, it does make the instructions more lengthy as drop a table would drop the index as well. I am fine with both way. This is mainly created to match to above PR.
There was a problem hiding this comment.
Thanx @adutra and @MonkeyCanCode for sharing the context. I was just curious if there is some functionality difference which I wasn’t aware. I am happy both ways, whichever everyone feels better
There was a problem hiding this comment.
In PSQL, there is none. Drop tables will drop indices (along with all other constraints). This PR was mainly to follow the same pattern that @adutra has in the single SQL PR.
IMO, if we don't want to support fall-back (down-upgrade), we should remove the drop index parts. If we do want to support some fall-back, the approach @adutra has is good. The only thing we may sign ourself up for is to keep those been implicit for other things such as relations etc. later on if certain tables has relations constraints and got dropped in the schema evolution.
|
|
||
| ```sql | ||
| DROP INDEX IF EXISTS polaris_schema.idx_idemp_realm_expires; | ||
| DROP INDEX IF EXISTS polaris_schema.idempotency_records; |
There was a problem hiding this comment.
The duplicate is gone, but the replacement now tries to DROP INDEX on idempotency_records, which is a table in schema v4. PostgreSQL rejects this even with IF EXISTS, so running the upgrade with ON_ERROR_STOP enabled stops before the remaining migration statements. Could we remove that line and keep the DROP TABLE below so the v4 upgrade runs without errors?
There was a problem hiding this comment.
Hello @flyingImer ,
Those were copied from https://github.com/adutra/polaris/blob/0668f5d5279e1cb367e16cc9991966d01352666b/site/content/in-dev/unreleased/metastores/relational-jdbc.md#migration-from-schema-v4-to-v5 (on-going PR from @adutra).
I hasn't try this on my setup but I can test it later this week. We should not merge this PR until the on-going one from @adutra is resolved. This is primary to match to what @adutra has in the on-going PR.
Thanks,
Yong Zheng
4 participants
Follow up on #5349 where the current SQL instructions missed the drop for table idempotency_records and its indices.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)