grate icon indicating copy to clipboard operation
grate copied to clipboard

Migration from roundhouse: Table names casing

Open cocytus opened this issue 2 years ago • 9 comments

Description Roundhouse uses lowercase table names, like "scriptsrun" in postgres due to the fact that the table name is not enclosed in "quotes". This means that when an existing postgres database created using roundhouse is updated using Grate, Grate considers no grate-schema to exist because of the casing differences (schema name is set correctly).

To Reproduce Create postgres db using roundhouse. Upgrade db using Grate with --schema roundhouse. It will create new tables like "ScriptsRun" along side the existing "scriptsrun" from RH.

Expected behavior An option to Grate to force lowercase table names?

cocytus avatar Sep 13 '22 13:09 cocytus

Wouldn't the work around here be to rename the objects to match Grate? After all, Roundhouse is essentially a 'different' product.

RachelAmbler avatar Sep 13 '22 15:09 RachelAmbler

Thanks for letting us know about this @cocytus!

We have had a few case-sensitivity related bug in different DBMS's, for different reasons. I personally only work with SqlServer (in a case-insensitive collation) so I've been slowly learning about all the ... interesting ... ways things handle case, including that quoting behaviour when I was looking into #230 the other week.

As part of that change the tests I added in #234 detected this issue wrt schema names, but not the tables... when I get a chance to surface for air in my day job I'll try and have a look at this one for you, unless you beat me to it 👍

While RoundhousE is a different product, and we're willing to break compat in some circumstances, grate is intended to be the spiritual successor, and for the most part should be broadly backwards compatible with a smooth upgrade path.

wokket avatar Sep 13 '22 22:09 wokket

Thanks a lot, @cocytus for reporting! The goal is indeed to be backwards-compatible with RoundhousE, and a drop-in replacement. As I don't work much with PostgreSQL on a day-to-day basis, what would you recommend as the preferred solution here?

  • Make grate also use only lower-case for our own migration table names
    • might be hard to change now, as it is breaking for any existing grate-users on PostgreSQL
  • Force renaming when migrating, as @RachelAmbler suggests?
    • I'm not very keen on this, as migration will be a bigger pain than expected
  • Handle this specially, or case-insensitive?
    • Would it be a fix to just remove the quotes around the grate table names for PostgreSQL?
    • Could this lead to any other potential issues that you know of?

This said, we DO have issues on case-sensitive collations for SQL server too, and we should look into proper case-sensitive support (probably accent-sensitive too, separating schema graté from grate. But this is a bigger job, I think.

erikbra avatar Sep 14 '22 21:09 erikbra

Hei!

  • Grate lowercase by default: Agree, any existing users of grate would have grate break when upgrading - no good!
  • Force renaming: Would make "testing" grate as a RH replacement riskier, because if you discover you have cases that only RH supports for some reason, you don't have an easy way back, Not that important maybe, and this is a viable path, but... not preferred?
  • Removing quotes: Would make lowercase the default for postgres as opposed to all other databases (?), and if there is existing users of grate-postgres combo, it would break.
    • Would it cause other issues? -> Not that I know of really, except that it looks like a bug. Table name is supposed to be "SomeThing", but it's "something" in db due to missing quotes in db name syntax, and this only applies to some db servers? Shady...

So, I believe my PR suggestion would be the lowest risk way forward. An alternative solution is to detect wherever the tables are in PascalCase or lowercase when starting (in AnsiSqlDatabase I would guess), and configure grate accordingly.

cocytus avatar Sep 15 '22 06:09 cocytus

Heisann, hoppsann! :)

I have been wondering and pondering about this a bit. How would the following work:

  • When we check if the grate tables and schemas exist, we always use case insentitive searching (without quotes or brackets or anything). Possibly just create custom-made, single purpose SQLs for this purpose
  • When we create the new tables and schemas, we use the correct casing

Have I missed some corner cases with this (to me, simple) approach, @cocytus ?

erikbra avatar Sep 20 '22 16:09 erikbra

That would work for all cases as far as I can understand. This was what I meant in my "alternative solution" mentioned above. I can create this unless you already have.

cocytus avatar Sep 20 '22 16:09 cocytus

I updated the PR with the "detect table casing" solution.

cocytus avatar Sep 21 '22 06:09 cocytus

Thanks a lot, @cocytus . I have looked at your PR with updates, and I think you have a lot of good stuff there! I personally would prefer to:

  • Skip the command-line parameter
  • Accept all variations of table casing, not just "the canonical" and lower-case

I have looked a bit into an alternative solution, here, what do you think? https://github.com/erikbra/grate/pull/256

erikbra avatar Oct 01 '22 21:10 erikbra

Hey,

Are you sure you're looking at the lastest version of my PR, the one I pushed 11 days ago? Because it has no command line parameter any more, it just detects the casing.

But sure, allowing any casing could also be done, as your PR solves. I have no strong feelings on what the best solution would be. Either would work in any real world scenario I'd think.

cocytus avatar Oct 02 '22 12:10 cocytus

Is this still on the cards to be merged in? Just run into the same thing with a migration from RoundhousE to grate - looks like for now we'll have to manually rename the tracking tables.

kieranbenton avatar Apr 26 '23 15:04 kieranbenton

@kieranbenton @erikbra had an alternative solution, but has not merged that either. Grate can still not drop-in replace roundhouse for postgres users..

cocytus avatar Apr 27 '23 20:04 cocytus

I have an open PR (#234) that is a partial fix wrt the schema name component, but that hasn't been merged either.

wokket avatar Apr 27 '23 21:04 wokket

Fixed in #256

erikbra avatar Jun 03 '23 19:06 erikbra

This is still not fixed. Running grate on a postgres database which had previously been migrated using roundhouse does not work.

cocytus avatar Jul 15 '24 10:07 cocytus