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

Driver name collision #151

Closed
Kaizer22 opened this issue Jan 12, 2022 · 14 comments · Fixed by #154
Closed

Driver name collision #151

Kaizer22 opened this issue Jan 12, 2022 · 14 comments · Fixed by #154

Comments

@Kaizer22
Copy link

Kaizer22 commented Jan 12, 2022

In our solution, we want to switch between mailru/go-clickhouse and native TCP ClickHouse driver ClickHouse/clickhouse-go using an environment variable for better compatibility and ease of development and deployment. But we can't do that because of the driver's name. They are the same and that's causes panic: sql: Register called twice for driver clickhouse.

Does anyone else think that this is an issue? Or maybe there is another solution?

@DoubleDi
Copy link
Collaborator

I think the best way to go here is build tags. Specify different libraries in different files and use tags.

@Slach
Copy link
Contributor

Slach commented Jan 13, 2022

@DoubleDi could you provide short example how to use mailru/go-clickhouse anc clickhouse/clickhouse-go on the same application on runtime?

@DoubleDi
Copy link
Collaborator

@Slach not sure that is possible. As both libraries register same driver name.

We can patch the lib, so it will support that using build tags or envs.

Why is it necessary to use 2 libs at the same time?

@Kaizer22
Copy link
Author

To be able to provide connection to a ClickHouse despite how and where it was deployed. For example:

We have ClickHouse at an external Kubernetes environment with Ingresses, configured only to provide an HTTP connection.

Also, we have an internal ClickHouse with prepared TCP ports, for connections using native driver.

And I, as a common connection provider library developer, want to deliver a flexible solution. To give an end-user opportunity to choose connection type on the fly, and not to worry much about environments' settings, using this library.

@Kaizer22
Copy link
Author

In the beginning, I've asked about that at StackOverflow and some of the community members agreed that using both of these libraries at the same time might be useful: https://stackoverflow.com/questions/70683405/choose-an-sql-driver-at-runtime-in-golang-when-drivers-have-the-same-name

@DoubleDi
Copy link
Collaborator

DoubleDi commented Jan 13, 2022

We can try to register the driver additionally by the name of chx or chhttp or ch. But there is still a problem when on the data marshaling part. For example when you send an clickhouse.Array you need to marshal it specifically for each lib. How do you handle that?

@Kaizer22
Copy link
Author

At least we'll be able to create an interface with methods, which will cover end-user needs without using specific data types and add two implementations that operate with different libs. Then we can provide a method that will return connection instances depending on the environment variable.

@kshvakov
Copy link

@Kaizer22 You can use V2 without database/sql

@myserg
Copy link

myserg commented Jan 28, 2022

@Kaizer22 You can use V2 without database/sql

No you can't. Your driver always calls sql.Register() standard library function.
@DoubleDi please just change your registered driver name to something like "clickhousehttp" and the problem will be solved for many of us.

@DoubleDi
Copy link
Collaborator

DoubleDi commented Jan 29, 2022

@myserg unfortunately, we cannot just change the name of the driver, as this would break all the current driver integrations.
Those are breaking changes.

One of the solutions is make a v2 driver version.
Another one is to add a specific tag chhttp to build your application with. On the library we will register the library as chhttp and add a tag // +build !chhttp to the place where the driver is registered as clickhouse.

The second solution looks simple. What do you think?

@myserg
Copy link

myserg commented Jan 29, 2022

It's just a key in the global unexported map, how it could possible break anything? (https://cs.opensource.google/go/go/+/refs/tags/go1.17.6:src/database/sql/sql.go;l=44). Anyway, I've just written a simple HTTP bulk writer, one less user for your driver.

@DoubleDi
Copy link
Collaborator

DoubleDi commented Jan 31, 2022

Most of the current users use this library as

c, err := sql.Open("clickhouse", "http://127.0.0.1:8123/default")

If we change the driver name to chhttp for example and bump an update, the code stated won't work as it will not find driver clickhouse. When the current users update the library version, their code will break. This is unacceptable for minor version updates.

This means that we can't change the name without bumping a major version of the driver. We can achieve that by introducing v2.

We can also try to introduce a hack using build tags, which I described. Is compiling your app with the chhttp is an acceptable solution for you?

Also if you have any other suggestions, that doesn't break the current user's experience let's discuss it or just make a PR.

@hanjm
Copy link

hanjm commented Mar 7, 2022

Hi, i am facing same issue, i think introducing v2 is the better way. the build tag way will limit the usage. Thank you.

@DoubleDi
Copy link
Collaborator

Please check if the current solution works for your usecase

@DoubleDi DoubleDi reopened this Mar 27, 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 a pull request may close this issue.

6 participants