nette / database

💾 A database layer with a familiar PDO-like API but much more powerful. Building queries, advanced joins, drivers for MySQL, PostgreSQL, SQLite, MS SQL Server and Oracle.
https://doc.nette.org/database
Other
513 stars 108 forks source link

PostgreSQL reflection across the schemas #17

Closed milo closed 10 years ago

milo commented 10 years ago

In dibi issue, @JanRossler pointed out, that comparing table by its name may fail with multiple schemas. Imagine two tables with the same name in different schemas:

CREATE TABLE "one"."tab" ("id_one" integer NULL);
CREATE TABLE "two"."tab" ("id_two" integer NULL);

If you set SET search_path TO one, two you get by reflection:

Issue 1

dump($driver->getTables());
[
    0 => ['name' => 'tab', 'view' => FALSE],
    1 => ['name' => 'tab', 'view' => FALSE],  # duplicit
]

Issue 2

dump($driver->getColumns('tab'));
[
    0 => ['name' => 'id_one', 'table' => 'tab'],
    1 => ['name' => 'id_two', 'table' => 'tab'],  # should not be there
]

One of the solutions is to use table_name::regclass (you get OID of the first table relatively to search_path) instead of schema name + table name comparision. It has side effect, using ::regclass on undefined table leads to SQL error. It is BC break because now you get empty result silently.

I am prepared to update PostgreSQL driver reflection part, but:

hrach commented 10 years ago

This is my concept how should it work (dunno it what exactly you are fixing/proposing)

We should fetch all relationships & tables, etc. with its schema (if it is not a primary schma) and store it as a string. (this is probably what you are fixing)

dump($driver->getTables());
[
    0 => ['name' => 'tab', 'view' => FALSE],
    1 => ['name' => 'two.tab', 'view' => FALSE],
]
milo commented 10 years ago

I suppose, we are talking about reflection for Database\Table\*... So, can be a table name written by FQN always anytime? I mean, you can write author or public.author in app anytime you wish. If no, reflection for two.tab is useless. If yes, all possible variations should be in getTables(), beacause of Database\Structure caching and afaik Database\Table\* should be updated to support FQN.

One note to caching... should cache depend on search_path PostgreSQL runtime? It is edge case.

Issues is related to nette/nette#1101 a little bit.

hrach commented 10 years ago

So, can be a table name written by FQN always?

Sorry, but "can" vs. "always" doesn't make sence to me :) Yes, you can write author or public.author in app anytime you wish.

If yes, all possible variations should be in getTables()

I can imagine, it's not necessary. Sth like set search path could be implemented. Conventions would allways return full paths.

One note to caching... should cache depend on search_path PostgreSQL runtime? It is edge case.

Probably yes; there could be sth like $conventions->setSearchPath which would internally also set that for db driver.

milo commented 10 years ago

Sorry, but "can" vs. "always" doesn't make sence to me :)

Late hour typo ;)

Yes, you can write author or public.author in app anytime you wish.

Sounds nice. Unfortunately, I cannot imagine all needed code changes to support this :(

If yes, all possible variations should be in getTables()

I can imagine, it's not necessary. Sth like set search path could be implemented. Conventions would allways return full paths.

It reimplements automatic PostgreSQL feature, but I understand why it is needed. Maybe some clever SHOW search_path could be used instead...

hrach commented 10 years ago

Still I don't fully understand what #18 do, or, if it's compatible with this concept. If I get it, it is all ok. current_schemas probably would return all schemas defined in search_path, am I right?

milo commented 10 years ago

Try to run this test from PR on current master and walk through the failed assertions.

Imo it is compatible, it is only the fix since current reflection implementation doesn't support schemas.

current_schemas probably would return all schemas defined in search_path, am I right?

Yes, the SELECT unnest(current_schemas(FALSE)) do the thing.