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

No explicit USE database statement #54132

Merged
merged 4 commits into from
Jan 9, 2025
Merged

Conversation

TheLevti
Copy link
Contributor

@TheLevti TheLevti commented Jan 9, 2025

No need to explicitly send a USE database statement. The database is already selected via the DSN string.

Multiple reasons for this:

  • One less round trip per connection
  • No connection pinning when used with a proxy (e.g. RDS Proxy).
  • No other drivers are doing that (again, database is already selected via the DSN string).

No need to explicitly send another `USE database` statement. The database is already selected via the DSN string.

Multiple reasons for this:
- One less round trip per connection
- No connection pinning when used with a proxy (e.g. RDS Proxy).
Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot be removed. It's necessary when persistent connections are enabled. It's also necessary for other MySQL compatible databases.

@crynobone crynobone marked this pull request as draft January 9, 2025 12:36
@crynobone crynobone marked this pull request as ready for review January 9, 2025 12:37
@TheLevti
Copy link
Contributor Author

TheLevti commented Jan 9, 2025

This cannot be removed. It's necessary when persistent connections are enabled. It's also necessary for other MySQL compatible databases.

Can you elaborate why this is needed for persistent connections? Also, which other mysql databases? If they are mysql compatible, then I would assume they also allow the database to be specified in the DSN.

We currently have the problem that we first of all need to get rid of any unnecessary round trip to the DB and also avoid connection pinning via RDS proxy. This non configurable behaviour forces us to overwrite this class or use a custom driver in all services. It would be nice if this can be solved directly on the framework. Same as all the other optional SET operations (timezone, mode, charset etc.)

Based on docs I have found, if the database is specified in the DSN, PDO takes care of ensuring that the right DB is selected (even for persistent connection). So it is not necessary to send out another USE statement on application level.

We have been using persistent connections for quite some time (with cakephp, there no explicit USE call is done), and we never had to send an explicit USE statement. (We use AWS Aurora MySQL + MySQL locally).

Proposal:

To be 100% backwards compatible just in case (if you don't trust it to be like I described), allow setting a new config value to skip setting the database explicitly.

Edit:
Even chatgpt o1 mentions this:

Short Answer: If you include the database name in your DSN (for example, mysql:host=localhost;dbname=your_database), PDO will internally issue a USE your_database statement whenever it (re)establishes a persistent connection, so you generally do not need to issue USE $database yourself. However, if you do not specify dbname=... in your DSN, then you should issue USE $database after you connect in order to guarantee you are using the correct default schema—persistent connections can be re-used in unpredictable states.

@taylorotwell taylorotwell merged commit d5aceb8 into laravel:11.x Jan 9, 2025
37 of 38 checks passed
@GrahamCampbell
Copy link
Member

I'd rather see this in 12.x as a breaking change.

@GrahamCampbell
Copy link
Member

In my experience, this is pretty unreliable, and this may cause subtle bugs for people.

@GrahamCampbell
Copy link
Member

oh, nvm. i see config got added, and it defaults to the same behiavour. :P

@TheLevti TheLevti deleted the patch-1 branch January 9, 2025 23:20
@TheLevti
Copy link
Contributor Author

TheLevti commented Jan 9, 2025

Thanks guys! This saves us some work.

@it-can
Copy link
Contributor

it-can commented Jan 10, 2025

@TheLevti maybe add a config option to the docs/config files describing this option?

@TheLevti
Copy link
Contributor Author

@TheLevti maybe add a config option to the docs/config files describing this option?

Will do that.

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