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

Change initStatements to charset option when create db connection. #4173

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

forfin
Copy link

@forfin forfin commented Aug 28, 2024

*When Openmage start. It seems to merge every etc/*.xml config including this file. So before this commit, end-user who want to remove initStatements must explicitly set empty to <initStatements /> in later file. Otherwise it still be there.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

 - This options pass to default Zend_Db_Adapter_Pdo_Abstract when init PDO
@kiatng
Copy link
Contributor

kiatng commented Aug 29, 2024

Have you put this in production? What are the effects/benefits with or without this PR?

@forfin
Copy link
Author

forfin commented Aug 29, 2024

Yes, I have used it in production without breaking anythings.

I found this because I was thinking that SET NAMES utf8 statement cause connection pinning in my AWS RDS Proxy env. but seems it not the case.

So, this commit did not breaking anythings and did not solve my original problem either. But 1 lesser query on every connections might be a beneficial for overall.

@kiatng
Copy link
Contributor

kiatng commented Aug 30, 2024

Ref https://dev.mysql.com/doc/refman/8.4/en/charset-connection.html

SET NAMES 'charset_name' [COLLATE 'collation_name']

SET NAMES indicates what character set the client uses to send SQL statements to the server. Thus, SET NAMES 'cp1251' tells the server, “future incoming messages from this client are in character set cp1251.” It also specifies the character set that the server should use for sending results back to the client. (For example, it indicates what character set to use for column values if you use a SELECT statement that produces a result set.)

I also read the referenced stackoverflow. I am not sure how it related to our use case. UTF8 has been default in OpenMage since the beginning and we don't have any issues mentioned in the stackoverflow such as threat and wrong encoding in client.

I propose to close this PR unless there is a more compelling reason.

@forfin
Copy link
Author

forfin commented Aug 30, 2024

Sorry that I give bad reference.

It start when Openmage create new PDO in Zend_Db_Adapter_Pdo_Abstract

  • When init new PDO($dsn, ...); $dsn string are produce by config xml files.
  • Before this commit $dsn was
$dsn = "$mysql:model=mysql4;initStatements=SET NAMES utf8;type=pdo_mysql;host=mysql;dbname=openmage;active=1";
  • After this commit $dsn will be
$dsn = "$mysql:model=mysql4;charset=utf8;type=pdo_mysql;host=mysql;dbname=openmage;active=1";
  • PDO dsn valid options are host, port, dbname, unix_socket, charset for MySQL
  • PDO dsn ignore non-existing option so initStatements is ignore.
  • charset=xxx option is equivalent to SET NAMES xxx .

So when no initStatements was given, 1 extra query is not call.

if (!empty($configArr['initStatements']) && $conn) {
$conn->query($configArr['initStatements']);
}

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.

3 participants