Hi all, I agree with Ayush. For the record, I initially included a DROP INDEX statement in my own PR [5349], but in hindsight, I think that was a bad idea that sparked this whole discussion. I apologize for that.
> #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 [...] the durable fix is a test per backend +1000. That is my intention as soon as [5349] gets merged. I think we can also improve on the visual presentation and propose migration steps for each database type, using Docsy tabbed panes [1]. But that PR is already big, so I think we need to move forward incrementally here. Thanks, Alex [1]: https://www.docsy.dev/docs/content/shortcodes/#tabbed-panes [5349]: https://github.com/apache/polaris/pull/5349 On Tue, Sep 22, 2026 at 6:30 AM Jean-Baptiste Onofré <[email protected]> wrote: > > 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 > >
