ariga / atlas

Manage your database schema as code
https://atlasgo.io
Apache License 2.0
5.68k stars 252 forks source link

Declarative workflow breaks on YugabyteDB #3057

Open lalloni opened 3 weeks ago

lalloni commented 3 weeks ago

YugabyteDB default index access method is "lsm".

It is also required for primary key indexes.

Assuming a schema including any index like:

CREATE INDEX foo_bar_idx ON foo (bar_id);

Or a primary key like:

CREATE TABLE foo
(
    bar VARCHAR PRIMARY KEY,
);

During the automatic schema inspection phase, Atlas figures it needs to include USING lsm into either CREATE TABLE or CREATE INDEX statements like this:

CREATE INDEX "foo_bar_idx" ON "foo" USING lsm ("bar_id");

And this:

CREATE TABLE "foo" (
  "bar" character varying NOT NULL,
  PRIMARY KEY USING lsm ("bar")
);

Then, when trying to execute the statements, it fails like this:

Error: create "foo" table: pq: syntax error at or near "lsm"

I guess the SQL parser chokes on the "lsm" index access method because it does not know anything about it, given it does not exist in PostgreSQL.

giautm commented 3 weeks ago

Hey, thank you for reporting this issue. But I have checked their document for syntax. It looks like Atlas still generates the correct syntax for lsm

They don't mention anything special about the lsm method, and it looks like USING lsm should be valid to use.

lalloni commented 3 weeks ago

It should be valid, I agree.

Since the error message mentions it, is it possible that lib/pq could be rejecting it?

lalloni commented 3 weeks ago

However, in YugabyteDB, lsm is the default index type, so there is a possible workaround on Atlas by not including the USING clause when the DB is Yugabyte, and the inspected or specified index type is lsm.

I quickly tested that, changing Atlas in a few places, and it seems to work.

Please take a look at https://github.com/ariga/atlas/commit/8cce007be24e195f43f9142266041deb40131522

If adding such a change to Atlas makes sense, I can improve the patch as needed and post a PR here.

a8m commented 3 weeks ago

Thanks for the suggestion, @lalloni. We currently do not have official support for YugabyteDB, but it's on our roadmap.

Since this is duplicate of https://github.com/ariga/atlas/issues/2420, I think we can close this issue and track the progress there.

lalloni commented 3 weeks ago

Thanks, @a8m.

I saw #2420 before, but it had no detail, plan, or code; it was just an unanswered question. So, I moved forward to making some specific progress here.

I'm not sure how to take your last comment in the context of the patch I shared earlier.

Should we pursue the idea of getting it in as a small change that at least makes it work?

And again, I can offer time from my side to improve it in any way you believe it should be improved to get it integrated back.

a8m commented 3 weeks ago

Adding support for a new database/driver is a quite complex process on our end. It requires us to integrate it into our CI pipelines and commit to supporting it across all types and syntax — including all our non-OSS parts. In addition, YugabyteDB does not support other PostgreSQL features that require more changes, and I believe an implementation for such driver should be different and not change the Postgres one.

Also, our HCL cannot contain unsupported options, and Yugabyte would need to have different file extension in our editor plugins (VSCode, JetBrains), similar to what we have for MariaDB or MySQL.

Currently, this is not on our upcoming tasks, as we haven’t received any customer requests for it. However, I believe we will eventually add it, and we may reach out to the Yugabyte team before we’ll start to work on it.

Thank you for your suggestion, but I'll decline it at this stage.