Hi guys, +1 to what Ayush said: I would keep the upgrade as a single DROP TABLE IF EXISTS polaris_schema.idempotency_records.
The decisive argument for me is that the block is documented as covering "an existing v3/v4 database". One snippet, two source versions, so every statement in it has to be a no-op on the lower one. v3 never had idempotency_records (it is created in the v3 -> v4 section), so ALTER TABLE ... DROP CONSTRAINT idempotency_records_pkey fails there, and with ON_ERROR_STOP the rest of the "migration" never runs. DROP TABLE IF EXISTS is a no-op on v3 and correct on v4. That property is worth more to operators than explicitness. On the revert motivation: drop statements do not record how to recreate anything. A "downgrade" needs the CREATE statements, and those already exist in schema-v4.sql and are repeated in the v3 -> v4 section of #5349. Listing more drops adds nothing there imho. If we want documented downgrade later, it should be its own script of CREATE statements. Also, idempotency_records_pkey is just the PostgreSQL default constraint name. A deployment where the constraint was created under a different name fails the ALTER and succeeds on the DROP TABLE. We claim PostgreSQL, CockroachDB and H2 for this block, so I would avoid generated names in operator facing SQL. As a convention, Ayush carve-out is the right line: list a dependency only when the drop reaches outside the object being removed, typically a foreign key from a table we are keeping, where CASCADE silently alters that retained table. That is an operator visible side effect and belongs in the instructions. Cascades confined to the dropped object are noise. One broader point while we are here: #5349 removes the runtime fallback and makes the migration mandatory with a fail fast on version mismatch. That turns these snippets from advisory into load bearing, and today they are untested prose (which is how a statement that errors on PostgreSQL got as far as it did). I think the durable fix is a test per backend: bootstrap schema-vN, apply the documented N -> N+1 SQL, assert the result matches schema-v(N+1). That would catch this class of issue in CI rather than on the list. Happy to open an issue for it. Regards JB On Tue, Sep 22, 2026 at 4:16 AM Yong Zheng <[email protected]> wrote: > Hello all, > > I would like to start this discussion regarding if we should list out > cascading dependency deletion during schema version change (see sample > reference in https://github.com/apache/polaris/pull/5437). > > With the sample reference, we will be dropping table > polaris_schema.idempotency_records which has index > "polaris_schema.idx_idemp_realm_expires" and PK on > "idempotency_records_pkey". > > When we drop the table, it would drop the PK along with index during > cascading dependency deletion. One reason to keep those is to easy the > revert process if we need to do so. However, keeping all of the > dependencies listed out is a bit tedious and add over-head for regular > schema upgrade (in this case, instead of running one drop sql, we would > need to do 2 drop plus 1 alter table sql statements). Thoughts? > > Thanks, > Yong >
