EnterpriseDB / mysql_fdw

PostgreSQL foreign data wrapper for MySQL
Other
533 stars 163 forks source link

Duplicate ENUM names not handled correctly #197

Closed davidfetter closed 2 years ago

davidfetter commented 4 years ago

When multiple tables use the same name but not the same definition for an ENUM type (yes, I get that that's not great naming),IMPORT FOREIGN SCHEMA suggests multiple incompatible type definitions.

Is there some way to prefix the suggestion of the type name with the name of the table, which would then be used both in the suggested type definition and in the foreign table creation script?

Better still, is there some way to generate the ENUM types in a first pass and then use them in creating foreign tables in IMPORT FOREIGN SCHEMA?

surajkharage19 commented 4 years ago

Thanks, @davidfetter for reporting this and suggestion on how to fix same enum column name in multiples tables.

If I understand correctly, you have raised the two issues here: 1: What if enum column name is same and different enum definitions for multiple tables in MySQL? 2: Why can't we just create those TYPES at first pass rather than emitting the NOTICE.

For 1, we can follow the suggestion by you which prefix the table name for enum type names. I can see Endi Caushi has posted a similar fix that will resolve our first issue.

For 2, ImportForeignSchema API expects the list of CreateForeignTableStmt. So, we can not return the CREATE TYPE statement from those FDW API.

Probable way -

I will more explore on this. Meanwhile, if you have any other thoughts then you can share. Also, the changes to fix the first problem looks good to me. Do you see any issues with that?

If we compare with postgres_fdw then it is expecting that those types should be there before we run IMPORT SCHEMA command otherwise it would fail with an error saying: type does not exist. The postgres_fdw document[1] says the same.

Thoughts?

[1] https://www.postgresql.org/docs/11/postgres-fdw.html#id-1.11.7.42.10.7

mrendi29 commented 4 years ago

Hi @surajkharage19. I just have started contributing to open-source projects. I like the option to emit the notice and build the CREATE TYPE query. Would that involve going through the schema tables to get all the enums that are part of the foreign schema and then write all the create type queries?

davidfetter commented 4 years ago

Emitting CREATE TYPE before the CREATE FOREIGN TABLE statements is a good bit friendlier to the person doing the import.

While the documentation does state that each element in the list "must contain a CREATE FOREIGN TABLE statement," it does not say that it cannot contain anything else. In this case, the "anything else" would be the CREATE statement(s) needed in order for the ensuing CREATE FOREIGN TABLE statement to succeed.

It is reasonable to discuss oversights in the API for PostgreSQL 14 on the pgsql-hackers mailing list, but that need not impede progress here.

surajkharage19 commented 4 years ago

Hi @surajkharage19. I just have started contributing to open-source projects. I like the option to emit the notice and build the CREATE TYPE query. Would that involve going through the schema tables to get all the enums that are part of the foreign schema and then write all the create type queries?

I don't think, we need to go through all the schema tables. mysqlImportForeignSchema() forming a query to fetch the table details for required tables only. And while going through each row, wherever we find enum type we can form the CREATE TYPE query there and let that query go through the parser and execute it. I don't find MySQL supports nested types so we can create type without any dependency on another type. Thoughts?

surajkharage19 commented 4 years ago

Emitting CREATE TYPE before the CREATE FOREIGN TABLE statements is a good bit friendlier to the person doing the import.

ok.

While the documentation does state that each element in the list "must contain a CREATE FOREIGN TABLE statement," it does not say that it cannot contain anything else. In this case, the "anything else" would be the CREATE statement(s) needed in order for the ensuing CREATE FOREIGN TABLE statement to succeed.

It is reasonable to discuss oversights in the API for PostgreSQL 14 on the pgsql-hackers mailing list, but that need not impede progress here.

Yeah, we can discuss this with PG community, but as you said, for now, I think my first approach seems working. Do you see any issues with that? I have generated the POC patch based on that approach. Please have a look at the attached enum.txt file and let me know your thoughts on same. This patch also includes the changes from Endi to fix the same enum name in multiple tables.

enum.txt

surajkharage19 commented 2 years ago

Hi,

We have fixed the above mentioned issue. We are closing the case for now. If the issue persists, you can reopen the case anytime.