Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration from roundhouse: Table names casing #245

Closed
cocytus opened this issue Sep 13, 2022 · 14 comments
Closed

Migration from roundhouse: Table names casing #245

cocytus opened this issue Sep 13, 2022 · 14 comments

Comments

@cocytus
Copy link

cocytus commented Sep 13, 2022

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?

@RachelAmbler
Copy link
Contributor

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

@wokket
Copy link
Collaborator

wokket commented Sep 13, 2022

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.

@cocytus cocytus mentioned this issue Sep 14, 2022
@erikbra
Copy link
Owner

erikbra commented Sep 14, 2022

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.

@cocytus
Copy link
Author

cocytus commented Sep 15, 2022

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.

@erikbra
Copy link
Owner

erikbra commented Sep 20, 2022

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 ?

@cocytus
Copy link
Author

cocytus commented Sep 20, 2022

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
Copy link
Author

cocytus commented Sep 21, 2022

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

erikbra pushed a commit that referenced this issue Oct 1, 2022
@erikbra
Copy link
Owner

erikbra commented Oct 1, 2022

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? #256

@cocytus
Copy link
Author

cocytus commented Oct 2, 2022

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.

@kieranbenton
Copy link

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.

@cocytus
Copy link
Author

cocytus commented Apr 27, 2023

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

@wokket
Copy link
Collaborator

wokket commented Apr 27, 2023

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

erikbra added a commit that referenced this issue Jun 3, 2023
* Solve #245 without new command-line parameter
* Ignore the standard tables casing issue for Oracle, as it has never been case sensitive
* Fixed casing of INFORMATION_SCHEMA in unit tests
@erikbra
Copy link
Owner

erikbra commented Jun 3, 2023

Fixed in #256

@cocytus
Copy link
Author

cocytus commented Jul 15, 2024

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

erikbra added a commit that referenced this issue Jul 22, 2024
…different casing for PostgreSQL and MariaDB> (#560)

There was a regression, a similar bug to #245 which was introduced when using SQL scripts to handle the grate table structure versioning (grate the grate), in release 1.7.0.  This should fix it.

Fixes #557
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants