Skip to content
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

[10.x] Alternative database port in Postgres DSN #46403

Merged
merged 2 commits into from
Mar 9, 2023
Merged

[10.x] Alternative database port in Postgres DSN #46403

merged 2 commits into from
Mar 9, 2023

Conversation

timvango
Copy link
Contributor

@timvango timvango commented Mar 8, 2023

PR #43542 has added support for alternative database names in the DSN for PostgreSQL.
This PR did not add support for an alternative port in the DSN, making it incompatible with the PgBouncer implementation of DigitalOcean (and most probably others).

This PR allows setting a different port that is used only for generating the PostgresConnector DSN, with the connect_via_port option. The port that should be used for the connect_via_database option should be specified here.

@timvango timvango closed this Mar 8, 2023
@timvango timvango changed the title [109.x] Alternative database name in Postgres DSN, allow pgbouncer aliased databases to continue working on 9.x [10.x] Alternative database port in Postgres DSN Mar 8, 2023
@timvango timvango reopened this Mar 9, 2023
@taylorotwell taylorotwell merged commit e7e9f36 into laravel:10.x Mar 9, 2023
@keithbrink
Copy link
Contributor

@timvango Lifesaver, thanks!

@timvango timvango deleted the feature/alternative-pgsql-dsn-port branch March 9, 2023 19:10
@startork
Copy link

Hi @timvango,
Would you be able to clarify why this is necessary? I currently am using a Digital Ocean Connection Pool without this config but worried something's going to crop up :)
The database config option is used for other things such as setting table_catalog in migration queries, hence the need for the connection_via_database option. However, I can't imagine the port is used for much else other than the dsn. So would setting the port to the Connection Pool port not achieve the same result?

@timvango
Copy link
Contributor Author

timvango commented Nov 7, 2023

Hey @startork,

I'm not entirely sure about the exact problem I ran into, but from what I can remember, the migrations were failing when I used a pooled connection, while they worked fine with a direct connection. Laravel already has the 'connection_via_database' option for situations like this, but DigitalOcean's PgBouncer implementation has a quirk: it uses different ports for pooled and direct connections. This causes an issue because trying to use the 'connection_via_database' option results in a connection rejection due to an invalid port for either the pooled or direct connection.

To work around this, I created a patch where I changed the connection port, and it seemed to fix the issue. I even used this patch in a production environment for a while, and it resolved the problems for me. Thinking that others might run into a similar situation, I decided to create this pull request. But it's possible that this issue might not be affecting many people. If everything is running smoothly on your end, then there's probably nothing to worry about.

@startork
Copy link

startork commented Nov 8, 2023

Hi @timvango,

When using connect_via_database I do understand that you sometimes have to change the port, I am using DigitalOcean and had to do the same thing.
However, this change suggests that laravel will sometimes use the port value and sometimes the connect_via_port value but I don't think that is ever the case.
In practice if connect_via_port is set then Laravel will only ever use that value, port becomes obsolete. I do think having some clarity around this is helpful and maybe some dev's will prefer to have a connect_via_port value to make it very obvious that that port is specific to pgBouncer; but, in the same vein, it should be clear that setting connect_via_port has exactly the same affect as changing the port value itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants