Add postgres schema support

[danieltdt]

Hello,
I've been using knex for many projects and I see that it didn't have a good support for postgres schemas. So, I created this pull request adding a new method .using which allows to scope table names within a schema (and adding the schema to the metadata queries).

Since many databases allows the "qualified name" structure for table names, I decided to put it into the generic objects builder and compiler, since it does not broken any API.

I added a few tests also and it seems to run fine (I don't have oracle/mariadb to run integration tests 😞 ).

So, do you guys think it is good to merge?

[tgriesser]

Thanks for the PR @danieltdt - going to wait on merging this right away to compare some of these changes with #316 - I was holding off on that because I wanted to make sure the API was the clearest possible. @tkellen you seem to utilize more of the full PG feature set than I, any opinions on either of these PR's.

[danieltdt]

I think both PR can be merged with small changes. This PR only includes .using and changes some methods to be aware of the current schema. The #316 adds more methods, like searchPath and hasSchema (the createSchema and dropSchema methods are already implemented on current master).
One thing that I would change is remove searchPath(newPath) from the schema API, since it uses the set command and, AFAIK, it is defined per connection. I would move it to the connection object, like:

{
  client: 'pg',
  connection: 'postgress://postgres@localhost:5432/knex',
  searchPath: 'myschema, public'
}

[tkellen]

Agreed 100% on setting the search path in the connection.

[danieltdt]

Added searchPath on config.

[meredian]

Any news on including schema support? About this, or #316 Issue

[danieltdt]

So... Any thoughts about it?

[bendrucker]

Sorry for keeping you waiting for so long. I appreciate your patience. The steps to getting this merged are:

• Reviewing Postgres-specific functions - schema management #316 and making sure this has parity w/ the schema functionality introduced there
• Rebasing to squash and remove the merge commit

Re #316, I'm not concerned w/ the schema builder methods for creating new schemas or modifying. Just want to make sure that anything that any good ideas with respect to using schemas introduced there are reflected here. I'll work on extracting the schema builder functionality from #316 separately.

[danieltdt]

Ok! I'll check this later at home.

[whitelynx]

I'd love to see this rebased so it can be merged.

[danieltdt]

lol I totally forgot about this PR 🙊
I'll try to rebase it now...

[standyro]

👍 seconded on getting this pull request rebased and in

[meredian]

Can be any help provided for branch merging? Waiting this patch already for 10 months :)

[rhys-vdw]

Hi @danieltdt. Looks like there's a few people waiting on this. Are you still interested in getting it merged? If you can satisfy @bendrucker's requests, I'll accept the PR.

[danieltdt]

Sorry about the delay, guys... The codebase of knex has changed deeply and the rebase is not so simple as I expected. I had given up this PR but, since people seems interested on it, I'll try to rebase it on this weekend.

[danieltdt]

Please, take a look and let me know if it is good to merge :)

[rhys-vdw]

Hi @danieltdt, thanks for coming back to this.

I am primarily focused on Bookshelf, so I do not know the internals of knex very well. Also I've never worked with multiple schemas before in Postgres, but I checked the docs and the concept is simple enough. Your changes look very clean superficially, and the tests seem good, but beyond that I can't really comment on the correctness of the implementation. I'm happy to defer to your judgement.

Sorry I didn't mention this in the last comment, but I think this really requires documentation. Would you be able to also update index.html?

Also, I'm not sold on the method name using. USING is already a keyword in PostgreSQL - but seemingly not in the case of specifying schema(?). I think schema() or something similarly explicit might be more appropriate. What are your thoughts?

[danieltdt]

Hey @rhys-vdw,
When I was implementing that, I really could not find a better name for that method xD
My first thought was schema() but I think knex.schema.schema('myschema').table(...) would be a bit strange... So I thought about using, but it is already a keyword in postgres (and all others DBMSs too) but it is already implemented on knex (https://github.com/tgriesser/knex/blob/master/src/query/joinclause.js#L45) under a specific API.
I decided to keep using method because I could not find out any other meaningful name for that. If you think schema() is good for knex API, I'm ok renaming it.
Once we decide the method name, I will update the docs.

[rhys-vdw]

I think knex.schema.schema('myschema').table(...) would be a bit strange...

True. And also would it not conflict with knex.schema as it is on the QueryBuilder also?

I still fear using is too ambiguous. When scanning the documentation I think I'd be looking for 'schema'. Also, it may be that USING keyword needs to be added to QueryBuilder for some reason in the future.

However, I can't think of anything sufficiently terse. usingSchema or withSchema perhaps? Too awkward?

[danieltdt]

knex.schema use SchemaBuilder for DDL commands. QueryBuilder is only for DML queries. They should never conflict.

Maybe withSchema is a good option! May I rename to it?

[rhys-vdw]

@danieltdt: Yes, please. :-) Sorry for the slow response.

[danieltdt]

@rhys-vdw Done! (sorry about slow commits 🙈)

[rhys-vdw]

Looks solid. Nice one. 👍