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

Use getSchemaTableName also in the create table statement #8006

Merged
merged 1 commit into from Sep 2, 2022

Conversation

mwgmnn
Copy link
Contributor

@mwgmnn mwgmnn commented May 16, 2022

When all other sql statements are built, this method is used for including the schema name in front of the table name (if specified). So to make it more consistent, it would be better to also create the table in the specified schema.

PS: Please indulge me for not opening an issue, as i think this optimization is pretty trivial and need no big discussion.

Signed-off-by: Michael Weigmann michael.weigmann@hsh-berlin.com

When all other sql statements are built, this method is used for including the schema name in front of the table name (if specified). So to make it more consistent, it would be better to also create the table in the specified schema.

PS: Please indulge me for not opening an issue, as i think this optimization is pretty trivial and need no big discussion.

Signed-off-by: Michael Weigmann <michael.weigmann@hsh-berlin.com>
@gregw gregw requested a review from janbartel May 24, 2022 23:13
@gregw
Copy link
Contributor

gregw commented May 24, 2022

I think CI failure was an overload of the CI machine. Re-running...

@janbartel
Copy link
Contributor

@mwgmnn so prior to this change, is it true that the table would not have been found as part of the schema, and so would have been recreated, but outside the schema? Does this change mean that a table that was existing outside of the schema will now be recreated within the schema? Is there likely to be any data loss because of that?

@mwgmnn
Copy link
Contributor Author

mwgmnn commented Jun 7, 2022

Hi @janbartel, it depends on what the default schema for the user is and what schema name is set. In oracle for example the table is created in the users own schema (same name as user), when you don't specify another schema name, while in mssqlserver the table is created by default in schema 'dbo'. So the metaData.getTables call in prepareTables would find the table only in oracle when you specify the user as schema name (which might be relatively common besides from not using a schema name at all).

If the table already exists, the driver/db throws an sql exception. So i think others should have run into this problem too, when they use schema names that differ from the default schema. If on the other side the jdbc session storage was already functioning nothing changes, as the check for the existing table isn't modified by this merge request. The table is already inside the right schema and a data loss shouldn't occur. The change only opens the possibility to use another schema than the default one in new installations.

@janbartel
Copy link
Contributor

@mwgmnn so IIUC: prior to this PR, if a user had specified a schema name that was different to the default schema for the database, then the table would have been created in the default schema. Thus all subsequent jdbc interactions would fail, because we use the fully qualified table name for those?

@mwgmnn
Copy link
Contributor Author

mwgmnn commented Jul 15, 2022

@janbartel Yes, that's correct. And to be more precise: the default schema is not fix for the database, it can be set for the user specifically, at least in MSSQLServer, or in Oracle you can execute something like ALTER SESSION SET CURRENT_SCHEMA = xyz;. So it is also very dbms specific.

@gregw gregw merged commit 1b78db7 into jetty:jetty-10.0.x Sep 2, 2022
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

Successfully merging this pull request may close these issues.

None yet

3 participants