Doc: Added missing instructions for schema upgrade from v4 to v5 in 1.7.0 doc

[MonkeyCanCode]

Follow up on #5349 where the current SQL instructions missed the drop for table idempotency_records and its indices.

Checklist

• 🛡️ Don't disclose security issues! (contact security@apache.org)
• 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
• 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
• 💡 Added comments for complex logic
• 🧾 Updated CHANGELOG.md (if needed)
• 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

[ayushtkn]

Thanx @MonkeyCanCode for the fix.

[ayushtkn]

This DROP TABLE is a duplicate of the one above the ALTER.

[MonkeyCanCode]

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

[ayushtkn]

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

	CREATE INDEX IF NOT EXISTS idx_idemp_realm_expires
	ON idempotency_records (realm_id, expires_at);

which we are dropping below

[adutra]

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.

[MonkeyCanCode]

Hi @adutra and @ayushtkn ,

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.

[ayushtkn]

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

[MonkeyCanCode]

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.

[flyingImer]

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?

[MonkeyCanCode]

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

[MonkeyCanCode]

Hello @flyingImer ,

I finally got a chance to review this carefully tonight. So the initial copied SQL from above PR is not correct. We have following DDL for this table on v4:

POLARIS=# \d idempotency_records
                   Table "polaris_schema.idempotency_records"
      Column      |            Type             | Collation | Nullable | Default
------------------+-----------------------------+-----------+----------+---------
 realm_id         | text                        |           | not null |
 idempotency_key  | text                        |           | not null |
 operation_type   | text                        |           | not null |
 resource_id      | text                        |           | not null |
 http_status      | integer                     |           |          |
 error_subtype    | text                        |           |          |
 response_summary | text                        |           |          |
 response_headers | text                        |           |          |
 finalized_at     | timestamp without time zone |           |          |
 created_at       | timestamp without time zone |           | not null |
 updated_at       | timestamp without time zone |           | not null |
 heartbeat_at     | timestamp without time zone |           |          |
 executor_id      | text                        |           |          |
 expires_at       | timestamp without time zone |           |          |
Indexes:
    "idempotency_records_pkey" PRIMARY KEY, btree (realm_id, idempotency_key)
    "idx_idemp_realm_expires" btree (realm_id, expires_at)

So the right SQL for 2nd index is following (as it is constraint):

ALTER TABLE polaris_schema.idempotency_records DROP CONSTRAINT idempotency_records_pkey;

cc @adutra , what do u think? Should we be detail on all of these or just drop table and let the backend DB handle the cascading?

Thansk,
Yong

[dimas-b]

From my POV I do not see any benefit in dropping the PK just before we drop the table 🤔

I assume all RDBMS systems should be able to cascade the table drop into PKs and indexes.

However, I'm fine with current SQL text (I assume it works 😅 )

[MonkeyCanCode]

Yes, drop the base table would cascade on the indices and constraints.

[dimas-b]

Also note that idempotency_records is expected to be empty. IIRC, the table was never used in released java code.

[MonkeyCanCode]

That is correct.

[dimas-b]

To clarify my view on this: I'd prefer to use only this DROP TABLE statement for simplicity.

However, current three-statement SQL also looks correct to me.

[MonkeyCanCode]

IMO. We should keep the DROP TABLE only for this case as the other two will be auto drop in this case. Doesn't make much sense for an admin to run 2 extra commands that is not must needed. However, i think we need to consolidated with @adutra on his PR. The positive side with list out all changes is if an admin need to reverted, he/she will know what were removed...If he/she only do create table (without creating index/constraint), that would be incomplete revert (assuming we want to support revert?)