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

MySQLi: Reuse existing connection #38

Closed
killerbees19 opened this issue Mar 21, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@killerbees19
Copy link
Contributor

commented Mar 21, 2017

It looks like Postfixadmin opens more than 45 new connections to the database for a single page without any real items. This is extremely slow, especially with external MySQL servers. Instead of using the existing connection, it creates a new one for each db_query, escape_string, …

There's a simple patch to fix this behavior: e28f3f5 (from this branch)

I've uploaded a backtrace without the patch here: pfa-backtrace.txt

@cboltz

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

Are you sure this still happens with the latest code we have on github?

Commit c253ef7 (done about a month ago) claims to have this fixed ;-)

That said - your fix looks better, so I seriously think about accepting your patch and reverting c253ef7 ;-)

@DavidGoodwin

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

My fix was only to allow a connection to be passed into the escape_string() function; this on the other hand short cuts the db_connect() function if a connection has been opened before. So they're slightly different.

There is already a static $link in db_query() ...

https://github.com/postfixadmin/postfixadmin/blob/master/functions.inc.php#L1419

@killerbees19

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

Are you sure this still happens with the latest code we have on github?

Nope, it's the Debian package from Stretch. Sorry, I didn't see the issue/commit. But I think it's a little bit different.

@killerbees19

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

There is already a static $link in db_query() ...

But it's useless when calling db_connect() from other functions like escape_string().

mysqli_connect() returns always a new connection:

(PHP 5, PHP 7)
mysqli::__construct -- mysqli_connect — Open a new connection to the MySQL server

@cboltz

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

Agreed, db_connect() is irrelevant for escape_string() - and caching the connection directly in db_connect() is probably the best thing we can do.

I still think that this fix is better than c253ef7 ;-)

@cboltz

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

I merged #41 (thanks!) and also reverted most of c253ef7 - there's no need for additional caching in escape_string().

@cboltz cboltz closed this Apr 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.