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.

Sign up for GitHub

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

Jump to bottom

[DBAL-920] Use PDO::PGSQL_ATTR_DISABLE_PREPARES by default #714

Closed
wants to merge 3 commits into from
Closed

[DBAL-920] Use PDO::PGSQL_ATTR_DISABLE_PREPARES by default #714

wants to merge 3 commits into from

Conversation

mbeccati
Copy link
Contributor

@mbeccati mbeccati commented Nov 2, 2014

@mvrhov
Copy link

mvrhov commented Nov 3, 2014

Don't you think this is overly aggressive?

@mbeccati
Copy link
Contributor Author

mbeccati commented Nov 3, 2014

how so?

@mvrhov
Copy link

mvrhov commented Nov 3, 2014

Well you are disabling all prepared statements on 5.6.

@mbeccati
Copy link
Contributor Author

mbeccati commented Nov 3, 2014

Yes, that's the point. I believe that the most common scenario is to execute each query only once. Always preparing the queries server side is a waste of time and resources.

@mbeccati
Copy link
Contributor Author

mbeccati commented Nov 3, 2014

Of course the change wouldn't be ok for a patch release, but worth considering for a major/minor.

Ocramius
Ocramius reviewed Nov 5, 2014
View reviewed changes
lib/Doctrine/DBAL/Driver/PDOPgSql/Driver.php
$this->_constructPdoDsn($params),
$username,
$password,
$driverOptions
);

if (PHP_VERSION_ID >= 50600) {
$pdo->setAttribute(\PDO::PGSQL_ATTR_DISABLE_PREPARES, true);
Copy link
Member

Choose a reason for hiding this comment

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

May still want to skip this block if $driverOptions includes something for \PDO::PGSQL_ATTR_DISABLE_PREPARES

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

@Ocramius Ocramius self-assigned this Nov 5, 2014
@Ocramius Ocramius mentioned this pull request Nov 5, 2014
Merged
@Ocramius
Copy link
Member

Ocramius commented Nov 7, 2014

@mbeccati I'm manually removing mbeccati/dbal@3c548dc: never merge master into a working branch (it causes hell when git bisecting afterwards), rebase instead.

Ocramius added a commit that referenced this pull request Nov 7, 2014
@Ocramius
#714 - stripping tabs (spaces instead)
dfd4dde
Ocramius added a commit that referenced this pull request Nov 7, 2014
@Ocramius
#714 - Adding a test verifying that attribute `PDO::PGSQL_ATTR_DISABL…
fb96454 
…E_PREPARES` is set when using pgsql on PHP 5.6+
Ocramius added a commit that referenced this pull request Nov 7, 2014
@Ocramius
#714 - Cleaning up test case (dedicated private method for skipped ca…
a576a3a 
…ses)
Ocramius added a commit that referenced this pull request Nov 7, 2014
@Ocramius
#714 - prepares should be disabled by default, but skipped when defin…
f11e14c 
…ed and not set to `true`
Ocramius added a commit that referenced this pull request Nov 7, 2014
@Ocramius
#714 - aligning setting PDO::PGSQL_ATTR_DISABLE_PREPARES with test …
1e130ff 
…specification
Ocramius added a commit that referenced this pull request Nov 7, 2014
@Ocramius
#714 - adding missing @group annotations for reference
d2db07c
Ocramius added a commit that referenced this pull request Nov 7, 2014
@Ocramius
#714 - db_type may not be defined, and db_name may be invalid
c66c41e
@Ocramius
Copy link
Member

Ocramius commented Nov 7, 2014

As of c66c41e I'm having tests for this issue, but it seems that PDO#getAttribute(PDO::PGSQL_ATTR_DISABLE_PREPARES) doesn't really cooperate -.-

@Ocramius
Copy link
Member

Ocramius commented Nov 7, 2014

See https://bugs.php.net/bug.php?id=68371 and https://bugs.php.net/bug.php?id=38182

Ocramius added a commit that referenced this pull request Nov 7, 2014
@Ocramius
#714 - ignoring PDOExceptions on PDO#getAttribute(PDO::PGSQL_ATTR_DIS…
d3f0d9f 
…ABLE_PREPARES) as per https://bugs.php.net/bug.php?id=68351
@deeky666
Copy link
Member

deeky666 commented Nov 7, 2014

The second is marked as Won't fix oO

@Ocramius
Copy link
Member

Ocramius commented Nov 7, 2014

Couldn't care less about their SNAFU :-)

@mbeccati
Copy link
Contributor Author

mbeccati commented Nov 7, 2014

I'll see what I can do about it (next week, currently at pgday 2014 ;) )

Ocramius added a commit that referenced this pull request Nov 7, 2014
@Ocramius
#714 - marking test as incomplete when affected by https://bugs.php.n…
cbf95ea 
…et/bug.php?id=68371
@Ocramius Ocramius closed this in ec6da75 Nov 7, 2014
@Ocramius
Copy link
Member

Ocramius commented Nov 7, 2014

Manually merged :-)

@mbeccati
Copy link
Contributor Author

mbeccati commented Nov 7, 2014

thanks :)

sarcher pushed a commit to sarcher/dbal that referenced this pull request Jan 1, 2015
@Ocramius @sarcher
doctrine#714 - stripping tabs (spaces instead)
1683171
sarcher pushed a commit to sarcher/dbal that referenced this pull request Jan 1, 2015
@Ocramius @sarcher
doctrine#714 - Adding a test verifying that attribute `PDO::PGSQL_ATT…
b032218 
…R_DISABLE_PREPARES` is set when using pgsql on PHP 5.6+
sarcher pushed a commit to sarcher/dbal that referenced this pull request Jan 1, 2015
@Ocramius @sarcher
doctrine#714 - Cleaning up test case (dedicated private method for sk…
33cb24a 
…ipped cases)
sarcher pushed a commit to sarcher/dbal that referenced this pull request Jan 1, 2015
@Ocramius @sarcher
doctrine#714 - prepares should be disabled by default, but skipped wh…
cef1dd4 
…en defined and not set to `true`
sarcher pushed a commit to sarcher/dbal that referenced this pull request Jan 1, 2015
@Ocramius @sarcher
doctrine#714 - aligning setting PDO::PGSQL_ATTR_DISABLE_PREPARES wi…
d39637e 
…th test specification
sarcher pushed a commit to sarcher/dbal that referenced this pull request Jan 1, 2015
@Ocramius @sarcher
doctrine#714 - adding missing @group annotations for reference
f52f025
sarcher pushed a commit to sarcher/dbal that referenced this pull request Jan 1, 2015
@Ocramius @sarcher
doctrine#714 - db_type may not be defined, and db_name may be inv…
fb3aa80 
…alid
sarcher pushed a commit to sarcher/dbal that referenced this pull request Jan 1, 2015
@Ocramius @sarcher
doctrine#714 - ignoring PDOExceptions on PDO#getAttribute(PDO::PGSQL_…
58a4f04 
…ATTR_DISABLE_PREPARES) as per https://bugs.php.net/bug.php?id=68351
sarcher pushed a commit to sarcher/dbal that referenced this pull request Jan 1, 2015
@Ocramius @sarcher
doctrine#714 - marking test as incomplete when affected by https://bu…
2640f3a 
…gs.php.net/bug.php?id=68371
@mbeccati mbeccati deleted the dbal-920 branch March 29, 2015 14:15
@doctrinebot doctrinebot mentioned this pull request Dec 6, 2015
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@Ocramius Ocramius

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mbeccati @mvrhov @Ocramius @deeky666