cockroachdb / cockroach

CockroachDB — the cloud native, distributed SQL database designed for high availability, effortless scale, and control over data placement.
https://www.cockroachlabs.com
Other
29.85k stars 3.77k forks source link

sql: names not quoted when reflecting FK constraint definitions using pg_get_constraintdef #111171

Closed gordthompson closed 11 months ago

gordthompson commented 11 months ago

Describe the problem

When CockroachDB reflects FK constraint definitions it does not quote names that PostgreSQL does.

To Reproduce

PostgreSQL

test=# select version();
                                                     version
------------------------------------------------------------------------------------------------------------------
 PostgreSQL 12.3 (Debian 12.3-1.pgdg100+1) on x86_64-pc-linux-gnu, compiled by gcc (Debian 8.3.0-6) 8.3.0, 64-bit

test=# DROP TABLE IF EXISTS "Unitéble2";
DROP TABLE
test=# DROP TABLE IF EXISTS unitable1;
DROP TABLE
test=# CREATE TABLE unitable1 ("méil" INTEGER NOT NULL, PRIMARY KEY ("méil"))
CREATE TABLE
test=# CREATE TABLE "Unitéble2" ("méil" INTEGER NOT NULL, "測試" INTEGER, PRIMARY KEY ("méil"), FOREIGN KEY("測試") REFERENCES unitable1 ("méil"));
CREATE TABLE

test=# select conname, oid from pg_catalog.pg_constraint where conname like '%fkey';
       conname       |  oid
---------------------+-------
 Unitéble2_測試_fkey | 16618

test=# select * from pg_catalog.pg_get_constraintdef(16618);
               pg_get_constraintdef
---------------------------------------------------
 FOREIGN KEY ("測試") REFERENCES unitable1("méil")

CockroachDB

> select version();
                                        version
---------------------------------------------------------------------------------------
  CockroachDB CCL v23.1.9 (x86_64-pc-linux-gnu, built 2023/09/07 16:22:15, go1.19.10)

> DROP TABLE IF EXISTS "Unitéble2";
DROP TABLE
> DROP TABLE IF EXISTS unitable1;
DROP TABLE
> CREATE TABLE unitable1 ("méil" INTEGER NOT NULL, PRIMARY KEY ("méil"))
CREATE TABLE
> CREATE TABLE "Unitéble2" ("méil" INTEGER NOT NULL, "測試" INTEGER, PRIMARY KEY ("méil"), FOREIGN KEY("測試") REFERENCES unitable1 ("méil"));
CREATE TABLE

>  select conname, oid from pg_catalog.pg_constraint where conname like '%fkey';
        conname       |    oid
----------------------+-------------
  Unitéble2_測試_fkey | 1075573307

> select * from pg_catalog.pg_get_constraintdef(1075573307);
              pg_get_constraintdef
-------------------------------------------------
  FOREIGN KEY (測試) REFERENCES unitable1(méil)

TL;DR

PG - FOREIGN KEY ("測試") REFERENCES unitable1("méil")
CR - FOREIGN KEY (測試) REFERENCES unitable1(méil)

This causes SQLAlchemy's regex to fail when it tries to reflect tables, for example

tt2= Table("Unitéble2", MetaData(), autoload_with=engine)
"""
Traceback (most recent call last):
…
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/dialects/postgresql/base.py", line 4163, in get_multi_foreign_keys
    m = re.search(FK_REGEX, condef).groups()
AttributeError: 'NoneType' object has no attribute 'groups'

"""

Expected behavior Name quoting consistent with PostgreSQL.

Environment:

Jira issue: CRDB-31802

blathers-crl[bot] commented 11 months ago

Hello, I am Blathers. I am here to help you get the issue triaged.

Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here.

I have CC'd a few people who may be able to assist you:

If we have not gotten back to your issue within a few business days, you can try the following:

:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

gordthompson commented 11 months ago

cc: @rafiss

rafiss commented 11 months ago

I ran the following in Postgres:

postgres=#  CREATE TABLE unitable1 ("méil" INTEGER NOT NULL, PRIMARY KEY ("méil"));;
CREATE TABLE
postgres=# CREATE TABLE "Unitéble2" ("méil" INTEGER NOT NULL, "測試" INTEGER, PRIMARY KEY ("méil"), FOREIGN KEY("測試") REFERENCES unitable1 ("méil"));;
CREATE TABLE
postgres=# select conname, oid from pg_catalog.pg_constraint where conname like '%fkey';;;
       conname       |  oid
---------------------+-------
 Unitéble2_測試_fkey | 41681
(1 row)

It looks like the result is the same as what CockroachDB returned.

Could you say a bit more about this part of the issue description? I don't think I know what this is referring to:

TL;DR

PG - FOREIGN KEY ("測試") REFERENCES unitable1("méil")
CR - FOREIGN KEY (測試) REFERENCES unitable1(méil)
gordthompson commented 11 months ago

The difference is that PostgreSQL's pg_get_constraintdef returns the quoted identifiers "測試" and "méil", while CockroachDB's pg_get_constraintdef does not include the quotes, just 測試 and méil

rafiss commented 11 months ago

Ah I see now. I didn't scroll down in the code block snippet so I didn't see the select * from pg_catalog.pg_get_constraintdef(16618); query.

rafiss commented 11 months ago

I don't fully understand why Postgres needs to quote those names. For example, defining the table this way works fine:

postgres=# CREATE TABLE "Unitéble3" ("méil" INTEGER NOT NULL, 測試 INTEGER, PRIMARY KEY (méil), FOREIGN KEY(測試) REFERENCES unitable1 (méil));

But then when showing the constraint, it still adds the quotes:

postgres=# select * from pg_catalog.pg_get_constraintdef(41706);
               pg_get_constraintdef
---------------------------------------------------
 FOREIGN KEY ("測試") REFERENCES unitable1("méil")
rafiss commented 11 months ago

I wonder if another way forward would be to make this regex broader: https://github.com/sqlalchemy/sqlalchemy/blob/aa7f15de35fa076afa2a7be2e3c3e030986f5a86/lib/sqlalchemy/dialects/postgresql/base.py#L4120-L4121

For example, it could be (?:"[^"]+"|[A-Za-z0-9_\S]+?). (That may have other unintended side effects, so if it's not possible, we should still see if we can change CockroachDB to add the quotes.)

gordthompson commented 11 months ago

Workaround implemented in sqlalchemy-cockroachdb:

https://github.com/cockroachdb/sqlalchemy-cockroachdb/pull/225