-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added PGSQL Support #111
Added PGSQL Support #111
Conversation
Remove call to boot which was causing recursive generation of ALL schemas.
Thanks for your contribution. Would you mind updating your branch with the latest changes, please? I'll wait for @gareth-ib's feedback and after that I'll merge your PR. |
src/Meta/SchemaManager.php
Outdated
if (! $this->hasMapping()) { | ||
throw new RuntimeException("There is no Schema Mapper registered for [{$this->type()}] connection."); | ||
} | ||
|
||
$schemas = forward_static_call([$this->getMapper(), 'schemas'], $this->connection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove these lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add them back. It's essential for the mapping process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except they render the code useless. When they are in, the code uses all schemas and generates all the tables from all the schemas, at least when using PostgreSQL. I tested it with and without on both and the functionality worked on both MySQL and PostgreSQL with the changes I made.
I'll do another round of testing, but I'm fairly certain I'll get the same result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's important to note that I am running the latest Laravel Nova, and the behavior I experienced is using that code base. I will check both code bases to see if there's a difference in the behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You wouldn't notice the difference unless PostgreSQL supports multiple databases in a single database deamon as MySQL does. For instance, you can have a MySQL database a
with a table a_table
and a database b
with a table b_table
having a foreign key pointing to a.a_table
. 😅 It's not easy to make a good example.
The key thing to generate proper relationships in our eloquent models would be being aware of cross-database relationships. I didn't find other way to achieve it.
… Original code seems to work fine.
I checked out the code, created an upstream reference to your repo, did a |
Postgres absolutely supports multiple databases in a single daemon... which
is probably what I saw, but the issue probably arose because I was using
an account that had Superuser access.
I will test again with multiple databases, with and without Superuser
access.
Cheers
Randy
…On Sun, Sep 2, 2018, 10:37 PM Cristian Llanos ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/Meta/SchemaManager.php
<#111 (comment)>:
> if (! $this->hasMapping()) {
throw new RuntimeException("There is no Schema Mapper registered for [{$this->type()}] connection.");
}
-
- $schemas = forward_static_call([$this->getMapper(), 'schemas'], $this->connection);
You wouldn't notice the difference unless PostgreSQL supports multiple
database in a single database deamon as MySQL does. For instance, you can
have a MySQL database a with a table a_table and a database b with a
table b_table having a foreign key pointing to a.a_table. 😅 It's not
easy to make a good example.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#111 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZbGd7bdDo49jFqVEzYuCeC5OTk-8wmks5uXKPxgaJpZM4WMfc2>
.
|
The issue isn't about the same type of cross-database querying that you can do with MySql. That functionality isn't inherently available in PostgreSQL, at least not up to 10.0, without the use of plugins. The issue is user rights and database isolation. PG allows each daemon access to all databases it knows about, and the only thing keeping an account from them is the role rights. In earlier versions, PG was less strict about enforcing those role rights. In PG 8.3, which I still have a couple old dev databases running on, when you execute:
It spins forever, opens many, many connections, and generates all models for all databases your user has access to with public schemas. When I do the same thing on 9.5, it isolates the user to the database and schema you've specified in your configuration. Very odd behavior, but it works normally in current 9.5 and later versions. |
Wow! What do you suggest, @rwdim? Should we do something about it? |
Where you say "it spins forever"... does it mean it never ends? 😅 'Cause if it does the job, I suppose, even if it takes too long, that would be OK? Obviously, there is room for improving efficiency 😅 but that might be subject for another PR. |
I've always stopped it 'cuz netstat shows 100+ connections... At any rate,
it only happens with PG8.x, so I wouldn't spend a moment fixing it. If I
was managing those databases properly (they're of little significance, so
upgrading them is not a priority), I'd have them on 10.x and would never
have seen the issue.
Bugs in prior major versions that can't be replicated in the current
version should be immediately closed, IMHO...
…On Tue, Sep 4, 2018 at 10:21 PM, Cristian Llanos ***@***.***> wrote:
Where you say it spins forever... does it mean it never ends? 😅 'Cause
if it does the job, I suppose, even if it takes too long, that would be OK?
Obviously, there is room for improving efficiency 😅 but that might be
subject for another PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#111 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZbGdm2VsbhWp0DVkXbFFVcm1pph3SVks5uX0M9gaJpZM4WMfc2>
.
|
Thanks 👍 |
Found one issue... I neglected to change the default table names to ignore... Should I do a new PR? Also started adding support for automatic Seeder and Migration support... |
It'd be better to add those features in different PRs, unless this PR needs them. |
Agreed.. Ill add them later under a new pr
…On Fri, Sep 7, 2018, 12:38 PM Cristian Llanos ***@***.***> wrote:
It'd be better to add those features in different PRs, unless this PR
needs them.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#111 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZbGQ1hklKIf9koTJ6qFX8dVJdUD8Enks5uYq8YgaJpZM4WMfc2>
.
|
Any chance for publishing this PGSQL support? |
As they don't seem to be accepting pull requests and maintaining. I've forked the repository and merged some of the pull requests. https://github.com/haakco/reliese-laravel-models https://packagist.org/packages/haakco/reliese-laravel-models |
@rwdim, thanks for all the effort you put into it. |
nah.. been down this road before.. my code works beautifully and has since
I did my PR.
cheers
…On Sun, Sep 2, 2018, 1:53 PM Cristian Llanos ***@***.***> wrote:
Thanks for your contribution. Would you mind updating your branch with the
latest changes, please? I'll for @gareth-ib <https://github.com/gareth-ib>'s
feedback and after that I'll merge your PR.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#111 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZbGc4DlFOEg5ewHP2n-pX9lUgT3v_gks5uXClCgaJpZM4WMfc2>
.
|
Added functional postgresql support to this already fantastic utility.