-
Notifications
You must be signed in to change notification settings - Fork 536
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
Create/Drop Firebird databases (issue #362) #365
base: main
Are you sure you want to change the base?
Conversation
…bedded and remote Firebird databases. Had to upgrade minimal version of FirebirdSql.Data.FirebirdClient to v5.0.0 in the FirebridSampleApplication. Tested with Firebird v3.0.3
…pt and return a list of statements (see issue DbUp#159).
- use unquoted table name and fix reference in generator and trigger
Failed to mention: Tested against Firebird 3.0.3 both in embedded mode and remotly. Had to increase the FirebirdClient to v.5.0.0. |
…atement type is no recognized.
…th updated sql statements in FirebirdTableJournal
The This might be a good case for splitting the providers into separate repos, then they can be versioned at different paces. |
@hhindriks Please rebase onto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a good idea to use the FbScript in the firebird provider. However it is not backwards compatible. E.g.. this does not work which is bad:
CREATE PROCEDURE FOO
AS
BEGIN
EXIT;
END
On the other hand this works:
SET TERM ^ ;
CREATE PROCEDURE FOO
AS
BEGIN
EXIT;
END^
SET TERM ; ^
Which is great because it does not work with the existing implementation.
Still I cannot approve that it is not backward compatible, We could consider making it optional (i.e. a classic vs. a FbScript mode)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should the medium-term implementation look like? Drop classic or co-exist both versions?
If drop-classic then we can mark it in version x as deprecated and remove it in version x+1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjauernig Well I just thought we could have both versions co-exist after all it is harsh to have many of peoples existing scripts stop working. My company would not like that anyway potentially having to change 100s of scripts because of a DbUp upgrade would be enough to not upgrade :) Perhaps we could change the default from classic to new at some point.
I will make a PR myself that uses @hhindriks approach since it will be easier than solving the conflicts. I will not fall back to using the entire script as he does but instead fall back to the classic approach when the new approach fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, both versions are fine.
See #773 on how to move this to the |
Implements DropDatabase and EnsureDatabase in the dpup-firebird project DbUp/dbup-firebird#1.
DropDatabase.For.FirebirdDatabase(connectionString); EnsureDatabase.For.FirebirdDatabase(connectionString);
Replaced the code copied from Postgres to split a script with a Firebird specific one as mentioned in #159.
Use unquoted table name for schemaversions table, in the generator and the trigger, they were inconsistent before.
DoesTableExistSql()
fixed (see #248 and #250). Firebird uses all uppercase for database objects unless they are quoted. Per above changed to unquoted names and using an UPPER(tablename) in the DoesTableExistSQL().