k3s-io / kine

Run Kubernetes on MySQL, Postgres, sqlite, dqlite, not etcd.
Apache License 2.0
1.56k stars 233 forks source link

fix(mysql): quote database name in CREATE DATABASE statement #149

Open dakraus opened 1 year ago

dakraus commented 1 year ago

In order to allow the full Unicode Basic Multilingual Plane, the database name is now quoted in the CREATE DATABASE statement.

fixes #148

brandond commented 1 year ago

Thank you for the contribution! Would you mind also fixing this for the other drivers as well?

dakraus commented 1 year ago

Sure @brandond, I will check the other drivers to see if we this fix is needed elsewhere.

dakraus commented 1 year ago

@brandond I added another commit to fix this for the PostgreSQL driver. However this also means that the database names for the PostgreSQL driver are now case-sensitive according to the official documentation:

Quoting an identifier also makes it case-sensitive, whereas unquoted names are always folded to lower case.

Another thing I noticed while running the tests locally: The CREATE DATABASE statement is never executed for CockroachDB and kine fails during the schema creation with if you configure a non-existing database in the KINE_ENDPOINT

[cockroachdb-v20.2] time="2022-11-29T12:51:42Z" level=info msg="starting metrics server path /metrics"
[cockroachdb-v20.2] time="2022-11-29T12:51:42Z" level=info msg="Configuring postgres database connection pooling: maxIdleConns=2, maxOpenConns=0, connMaxLifetime=0s"
[cockroachdb-v20.2] time="2022-11-29T12:51:42Z" level=info msg="Configuring database table schema and indexes, this may take a moment..."
[cockroachdb-v20.2] time="2022-11-29T12:51:42Z" level=trace msg="SETUP EXEC : CREATE TABLE IF NOT EXISTS kine ( id SERIAL PRIMARY KEY, name VARCHAR(630), created INTEGER, deleted INTEGER, create_revision INTEGER, prev_revision INTEGER, lease INTEGER, value bytea, old_value bytea );"
[cockroachdb-v20.2] time="2022-11-29T12:51:42Z" level=fatal msg="building kine: pq: no database or schema specified"
brandond commented 1 year ago

Thanks! I take it the sqlite driver didn't need any changes?

However this also means that the database names for the PostgreSQL driver are now case-sensitive.

That seems like a fair tradeoff; I'll make sure that is mentioned in the release notes.

The CREATE DATABASE statement is never executed for CockroachDB

Hmm, the logs suggest that it is - are you saying that it actually is not? Or that the CREATE TABLE statement just silently fails?

dakraus commented 1 year ago

Yes, the sqlite driver just works.

Regarding CockroachDB: ~The CREATE DATABASE statement is obviously executed (as shown in the logs) and the tests are also passing in Drone, so I guess that something is wrong on my end. If I can reproduce this behavior, I would open another issue for that.~ I have to correct my statement above - the CREATE DATABASE statement is actually not executed. I described this in #150.

dereknola commented 10 months ago

Is this still relevant?

brandond commented 10 months ago

It is, but I have been reluctant to merge it because of the change in behavior for Postgres.