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

Postgres:ListDatabases - add default database to connection #287

Closed
wants to merge 1 commit into from

Conversation

JFriel
Copy link
Contributor

@JFriel JFriel commented Jul 10, 2024

Issue raised in RDMP - HicServices/RDMP#1873
The issue appears as though when connection to a Postgres server, if no database is specified, it will Ngpsql will attempt to connect to a database with the same name as the user.

This fix uses the default postgres database to connect to in order to enumerate the databases on the server

@jas88
Copy link
Member

jas88 commented Jul 10, 2024

Is reverting to the #273 behaviour the best route here, or just using whatever database is in the connection string? We can't assume postgres exists either, but if we use the one the user specified that's defensible.

@JFriel
Copy link
Contributor Author

JFriel commented Jul 10, 2024

Is reverting to the #273 behaviour the best route here, or just using whatever database is in the connection string? We can't assume postgres exists either, but if we use the one the user specified that's defensible.

In the case of RDMP checking if the database exists before creating it, I suppose we could just catch the .Exists() call and if the error is the db does not exists error then assume it does not exist and we should create it

@@ -66,7 +66,7 @@ public override IEnumerable<string> ListDatabases(DbConnectionStringBuilder buil
//create a copy so as not to corrupt the original
var b = new NpgsqlConnectionStringBuilder(builder.ConnectionString)
{
Database = null,
Database = "postgres", //Use the default database, otherwise Npgsql will try to connect to a database with the same name as the user
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're actually best deleting this line, rather than assigning any value to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we delete this line it will try to connect to a db with the same name as the user connecting. Is that the behaviour we want?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it will connect to whatever db the user specified in the connection string, which is the only one we can reasonably rely on existing. We can't create databases at all on Oracle (it has a very "Oracle" approach to that!) and won't normally have the right permissions to do that on HIC or EPCC systems either, what matters is being able to connect reliably with the connection string provided. If this code fails with line 69 deleted, it means the user gave us an invalid connection string and it's obvious how to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, suppose the real issue is finding a clean way of dealing with the associated RDMP issue...

@JFriel JFriel closed this Jul 15, 2024
@jas88 jas88 deleted the bugfix/RDMP-204 branch August 17, 2024 19:10
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.

2 participants