Hi All, +1 to merge 5349 (I also approved in GH).
Cheers, Dmitri. On Tue, Sep 29, 2026 at 9:58 AM Alex Dutra <[email protected]> wrote: > Hi all, > > The PR [5349] has received two approvals already. Are we OK to merge it as > is? > > Thanks, > Alex > > [5349]: https://github.com/apache/polaris/pull/5349 > > On Sat, Sep 26, 2026 at 1:44 AM Yufei Gu <[email protected]> wrote: > > > > Hi Alex, > > > > Thanks for the update. The PR looks good to me now. > > > > Creating a document to list all source-of-truth schema files in the Git > rep > > works for now. We can address any further improvements as follow-ups. > > > > Yufei > > > > > > On Fri, Sep 25, 2026 at 2:28 PM Prithvi S <[email protected]> > > wrote: > > > > > Hi all, > > > > > > +1 on landing #5349 this way. The version check belongs with the single > > > schema.sql, and the upgrade notes can wait. > > > > > > The page still says to apply the migration SQL below, and that SQL is > gone. > > > What's left is a diff of schema.sql, from v6 on. > > > I think we should still give people some SQL. A schema diff shows the > end > > > state, not what to run when the tables already have data. > > > idempotency_records only exists in v4, and the v5 file says it was > never > > > wired up. v3 to v5 has nothing to do for that table. Running every hop > > > creates it and then drops it. On a real v4 database, DROP TABLE IF > EXISTS > > > is enough, index and primary key included. I'd only list a dependency > when > > > the drop changes some other table we're keeping, like Ayush said. > > > Same kind of gap elsewhere. The 1.7 events change needs an UPDATE to > clear > > > '__realm__', and a schema diff only shows the nullable column. The v6 > notes > > > drop and recreate idx_locations, and Cockroach needs indexes even when > > > version already says 6. #5349 checks that number, so the database still > > > starts. > > > #5437 already added the idempotency drop on the published 1.7.0 page. > The > > > changelog and the doc on main never picked it up. > > > So a short note per release, only for those steps. New tables and > columns > > > come from the diff. Anything added and removed in between can be > skipped. > > > > > > I'd keep the SQL as a file in the repo and test it the way JB > described. > > > Load the old schema, run the file, check columns and indexes against > the > > > next schema. The doc links the file. > > > > > > Thanks, > > > Prithvi S > > > > > > On Fri, Sep 25, 2026 at 6:53 PM Dmitri Bourlatchkov <[email protected]> > > > wrote: > > > > > > > Hi All, > > > > > > > > Alex's proposal for making progress here sounds good to me. +1 to PR > > > [5349] > > > > in its current state. > > > > > > > > I think it is also a good idea to reopen the discussion on schema > upgrade > > > > instructions after merging this PR. > > > > > > > > [5349] https://github.com/apache/polaris/pull/5349 > > > > > > > > Cheers, > > > > Dmitri. > > > > > > > > On Fri, Sep 25, 2026 at 9:03 AM Alex Dutra <[email protected]> > wrote: > > > > > > > > > Hi Yufei, hi all, > > > > > > > > > > In [5349], I do believe that the instructions were complete and > > > correct. > > > > > > > > > > However, in the spirit of making progress in that PR, which is > stuck > > > > > until we settle on this specific topic, I went ahead and removed > ALL > > > > > migration SQL snippets from that PR, **including** what was already > > > > > present on main, as you requested. Only generic instructions about > how > > > > > to get the schema diffs were retained. > > > > > > > > > > I hope this move will help get that PR unstuck. > > > > > > > > > > But now I think this discussion thread (or a new one maybe) should > > > > > answer a broader question: should we provide any form of specific > SQL > > > > > migration guidance to users, and if so, in what form? > > > > > > > > > > Thanks, > > > > > Alex > > > > > > > > > > [5349]: https://github.com/apache/polaris/pull/5349 > > > > > > > > > > > > > > > On Thu, Sep 24, 2026 at 10:53 PM Yufei Gu <[email protected]> > > > wrote: > > > > > > > > > > > > > That's a net loss for end users. > > > > > > > > > > > > This assumes the documented instructions are correct and > sufficient > > > to > > > > > > complete the upgrade. I don’t think that’s the case. For example, > > > what > > > > > > steps should users follow to upgrade from a schema version > earlier > > > than > > > > > v4 > > > > > > to v5? Why do users need to create a table and then delete it > during > > > a > > > > > > upgrade? It is getting even more complex when data backfill is > > > needed. > > > > > In a > > > > > > real env, users still need to consult the versioned schema > files,* > > > the > > > > > > source of truth*, to understand what has changed. > > > > > > > > > > > > Schema upgrades are critical operations that require great care; > > > even a > > > > > > small error can break the system. Given these gaps, I don’t think > > > users > > > > > can > > > > > > confidently rely solely on the Schema upgrades section > > > > > > < > > > > > > > > > > > > > https://polaris.apache.org/releases/1.7.0/metastores/relational-jdbc/#schema-upgrades > > > > > > > > > > > > . > > > > > > > > > > > > Maintaining separate migration instructions also adds a > maintenance > > > > > burden > > > > > > for the community when we already have the source of truth in the > > > rep. > > > > > > Besides, incomplete instructions can give users a false sense of > > > > > confidence > > > > > > that following the page is sufficient for a safe upgrade, which > is > > > > > > disastrous. > > > > > > > > > > > > Yufei > > > > > > > > > > > > > > > > > > On Wed, Sep 23, 2026 at 6:06 AM Alex Dutra <[email protected]> > > > wrote: > > > > > > > > > > > > > Hi Yufei, > > > > > > > > > > > > > > > This fails on older schemas that don't have the events table. > > > > > > > > > > > > > > If a user follows the migrations sequentially, the events > table is > > > > > > > guaranteed to exist by the time the v4 to v5 script runs. The > > > UPDATE > > > > > > > statement only fails if someone tries to apply the v4 to v5 > > > migration > > > > > > > on a schema that never went through previous steps. That's an > > > > operator > > > > > > > error, not a documentation issue. > > > > > > > > > > > > > > > I'd suggest removing the entire Schema upgrades section > > > > > > > > > > > > > > That's a net loss for end users. Why not keep it? I agree it's > > > > subject > > > > > > > to drift, but as I mentioned earlier, 1) it's still better than > > > > > > > nothing, and 2) we can prevent drift by creating migration > tests > > > that > > > > > > > exercise the migration steps. > > > > > > > > > > > > > > Thanks, > > > > > > > Alex > > > > > > > > > > > > > > On Wed, Sep 23, 2026 at 5:18 AM Yong Zheng <[email protected]> > > > > wrote: > > > > > > > > > > > > > > > > Thanks for the feedback all. I will proceed with the change. > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Yong > > > > > > > > > > > > > > > > On 2026/09/22 23:45:05 Yufei Gu wrote: > > > > > > > > > I think users shouldn't rely on SQL snippets in the upgrade > > > guide > > > > > as > > > > > > > the > > > > > > > > > source of truth for schema changes. > > > > > > > > > > > > > > > > > > For example: UPDATE polaris_schema.events SET catalog_id = > NULL > > > > > WHERE > > > > > > > > > catalog_id = '__realm__'; > > > > > > > > > This fails on older schemas that don't have the events > table. > > > > Like > > > > > the > > > > > > > > > ALTER TABLE example discussed above, it shows why migration > > > steps > > > > > need > > > > > > > to > > > > > > > > > account for the starting schema version. > > > > > > > > > > > > > > > > > > I'd suggest removing the entire Schema upgrades section > > > > > > > > > < > > > > > > > > > > > > > > > > > > > > https://polaris.apache.org/releases/1.7.0/metastores/relational-jdbc/#schema-upgrades > > > > > > > >. > > > > > > > > > The historical schema SQL files should remain the source of > > > truth > > > > > for > > > > > > > each > > > > > > > > > schema version. Maintaining migration SQL separately in the > > > > > > > documentation > > > > > > > > > is prone to errors and drift. > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Sep 22, 2026 at 10:40 AM Alex Dutra < > [email protected] > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
