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

Add doc for issue #8420 #14215

Merged
merged 11 commits into from
May 31, 2017
Merged

Conversation

ihipop
Copy link
Contributor

@ihipop ihipop commented May 26, 2017

#8420 (comment)

Q A
Is bugfix? no
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues #8420

@samdark samdark added the type:docs Documentation label May 26, 2017
@samdark samdark self-assigned this May 26, 2017
@samdark samdark added this to the 2.0.12 milestone May 26, 2017
@samdark
Copy link
Member

samdark commented May 28, 2017

@ihipop, @tom--, @beowulfenator please check docs in this pull request.

@beowulfenator
Copy link
Contributor

How do I add something to this PR?

@samdark
Copy link
Member

samdark commented May 28, 2017

You can't since you don't have write access to this repo. Either comment inline or, alternatively, access could be given.

@ihipop
Copy link
Contributor Author

ihipop commented May 30, 2017

@samdark Thank you for your great work.
But I have some different advice.

The table may remain locked by MySQL and cannot be written to by other queries. Thus, new connection should be created for this purpose.

This is not the purpose that we establish a new connection to MySQL server, we establish a new connection to MySQL server because :

Unless whole data set has been retrieved, no other query could be done through the same connection

Even we establish a new connection to MySQL server ,tables may still remain locked by MySQL and cannot be written to by other queriesBecause MySQL keeps locks on data in the case of MyISAM on the entire table ,and In Unbuffered Query Mode, because the loop done by PHP is much more slower than Buffered Query done by PDO (Not only because PHP is slower than C, but also the developer usually do some business logic in the loop for their specify reason), the lock time to table will much more longer than Buffered Query , these two reasons are too different.

So, The lock table statement should be moving to the warning part as I've commited.

Unbuffered Query is not a silver bullet indeed, just as what @SamMousa have said:

As I've said before, no solution is valid for all cases and therefore we should not just implement something. Instead implement it yourself as a custom function or behavior in your query class.

Maybe the total solution should be done by the developer in their specify business code, for us , a well document is on call.

@beowulfenator

@ihipop ihipop changed the title add doc for issue #8420 Add doc for issue #8420 May 30, 2017
@ihipop
Copy link
Contributor Author

ihipop commented May 30, 2017

And, also, I think that the paragraph about the memory strategy of PHP before 5.6 (which use mysqlnd) should not be removed
It explains that why batch() and each() used to seems to work (will not hit the memory limit problem, but will eat the server's memory at all) and lead a meditate to Bunffered Query

Note:
When using libmysqlclient as library PHP's memory limit won't count the memory used for result sets unless the data is fetched into PHP variables. With mysqlnd the memory accounted for will include the full result set.

Describe as: http://php.net/manual/en/mysqlinfo.concepts.buffering.php

@beowulfenator
Copy link
Contributor

Can you guys give me access to the repo?

@ihipop
Copy link
Contributor Author

ihipop commented May 30, 2017

@beowulfenator
Copy link
Contributor

Does this look better?

@samdark
Copy link
Member

samdark commented May 30, 2017

Looks great to me.

@samdark
Copy link
Member

samdark commented May 30, 2017

@ihipop how do you find it now?

@ihipop
Copy link
Contributor Author

ihipop commented May 31, 2017

@beowulfenator excellent work.
I would like to tweak a few words

@samdark samdark merged commit 137a936 into yiisoft:master May 31, 2017
@samdark
Copy link
Member

samdark commented May 31, 2017

Merged. Thank you!

@ihipop ihipop deleted the feature/add_doc_for_issue_#8420 branch May 31, 2017 08:26
@tom--
Copy link
Contributor

tom-- commented May 31, 2017

This is good. Thanks

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

Successfully merging this pull request may close these issues.

4 participants