Fix bug #80892 PDO::PARAM_INT on pdo_pgsql #6801

mbeccati · 2021-03-23T10:25:02Z

Created PR for review - I will commit to the appropriate branches when ready.

mbeccati · 2021-04-12T06:23:46Z

Merged in 340a067

taylorotwell · 2021-04-28T21:13:01Z

Hey all - we got a PR related to this change on Laravel: laravel/framework#37168

In summary, previously if you had a boolean column on a Postgres table, you could bind an integer value (0 or 1 typically) in the where condition against that column using PDO::PARAM_INT and everything was fine. However, you will now receive an error.

That's OK I guess, although fairly unexpected and large breaking change on a patch release?

In addition, if we try to adjust Laravel to bind all booleans using PDO::PARAM_BOOL that breaks applications that were previously doing something like querying a smallint column by binding a integer casted boolean value using PDO::PARAM_INT - since you can't bind a PDO::PARAM_BOOL binding when querying a smallint column. I don't think that is really PHP's problem because we were automatically casting booleans to integers when making Postgres queries and always binding using PDO::PARAM_INT.

But, it does put us in a pretty tough spot, since if we don't merge this PR change, people can no longer perform simple boolean queries in Laravel using PHP 8.0.5, while if we do merge it, we break all applications querying smallint columns via booleans - and we don't have a good way of determining what they are actually intended to do. Admittedly, it seems like querying a smallint column by passing true or false to represent 1 or 0 is not exactly the best approach to querying "boolean like" columns in Postgres, but people do it. 🤷

Ultimately, if this PR stands, we would just need to remove this convenience boolean to integer casting when working with Postgres and people could no longer pass booleans in our query builders when querying if a smallint column is 0 or 1 - and that's generally fine I guess - but just wanted to check with y'all first on if you consider this breaking change of not being being able to bind a PDO::PARAM_INT binding against a boolean column as too large of a breaking change to let pass on a patch release.

Any thoughts?

/cc @driesvints

nikic · 2021-04-28T21:41:50Z

ext/pdo_pgsql/pgsql_statement.c

-					if (PDO_PARAM_TYPE(param->param_type) == PDO_PARAM_LOB) {
+					if (PDO_PARAM_TYPE(param->param_type) == PDO_PARAM_INT) {
+						/* we need to check if the number requires bigints */
+						long long val = strtoll(Z_STRVAL_P(parameter), NULL, 10);


Without having looked into Taylor's issue yet, this code is clearly broken :( If the original value was null or bool, then it will stay as such, while this code assumes the value is always a string. Passing a null to a PARAM_INT binding causes a segfault now (bool doesn't for some reason).

Indeed I thought I had better coverage :(

mbeccati · 2021-04-28T21:47:55Z

Thanks @taylorotwell - I will have a better look tomorrow. Also cc-ing @derickr @carusogabriel as RMs

kamil-tekiela · 2021-04-28T22:42:47Z

Was this PR merged without a review?

mbeccati · 2021-04-29T05:59:57Z

@kamil-tekiela The bug report hit internals. The PR has been available for several days before I merged it in. In fact the strtoll usage itself had been reviewed and approved. In an ideal world everyone tests the RCs with their applications and this kind of BC breaks are intercepted.

In an ideal world also I wouldn't have shuffled the code out of the previous else block above thining "yeah, it's ok, tests are green", but sadly that happened.

That said, I'm currently working to fix the issue and making sure there are no regressions.

mbeccati · 2021-04-29T07:36:28Z

mbeccati@ff69c9e is my tentative fix. It surely does fix the segmentation fault and leaves booleans out of the actual change. I don't know however if I've fully covered Laravel's test scenario. TBH I'm a bit on the fence on this one and I would also consider reverting the change.

nikic · 2021-04-29T09:15:39Z

@mbeccati That patch looks reasonable as far as the segfault is concerned, but I don't think it would address Laravel's issue, which is the other way around: A boolean column addressed through PARAM_INT.

mbeccati · 2021-04-29T09:39:05Z

@nikic mbeccati@ebf1f5e

Unfortunately the new test fails due to:
SQLSTATE[42804]: Datatype mismatch: 7 ERROR: column "c" is of type boolean but expression is of type integer

The same test passes on 7.3.

Segfault aside, fixing the the PARAM_INT bug brings a significant and unexpected behaviour change that doesn't belong to a minor release.

If you agree, I will proceed with the revert.

nikic · 2021-04-29T09:41:58Z

@mbeccati Yeah, I think we should revert this from the stable releases. That would still leave the question of whether this change should be made for PHP 8.1, or whether this should be left alone altogether.

mbeccati · 2021-04-29T10:12:22Z

Reverted from 7.4 to master. I think the matter requires some more thoughts about pros and cons to apply a similar bug fix to 8.1+.

Thanks again @taylorotwell for reporting.

mvorisek · 2021-04-30T11:18:49Z

This PR has broken also atk4/dsql and atk4/data tests.

I fixed it in atk4/dsql#299 . For some reasons, PARAM_INT (with this fix) can not be used with boolean columns, but PARAM_STR can.

But this PR makes sense to me for PHP 8.1.

FYI PARAM_BOOL in Oracle should on the other side map to PARAM_INT, as PARAM_BOOL does not work, see atk4/dsql#299 (comment) and based on https://stackoverflow.com/questions/3726758/is-there-any-boolean-type-in-oracle-databases it is not even supported by Oracle

hansgv · 2021-05-02T20:00:50Z

@mbeccati According to https://www.php.net/ChangeLog-7.php this ended up in the 7.4.18 release. Was that intentional?

mbeccati · 2021-05-03T07:27:23Z

@hansgv Yes, it ended in 7.4.18 and 8.0.5... that's unfortunate. I wish more poeple would also test release candidates so that this kind of issues could be intercepted, reviewed and eventually rolled back before the final release.

mbeccati · 2021-05-03T07:33:06Z

With that said, I think the actual bug is in the software trying to bind an integer for a boolean column and I hope we'll have a chance to fix binding in 8.1, even if that means people need to adjust their own code to avoid depending on PHP bugs.

tbleckert · 2021-05-03T07:42:05Z

@mbeccati Will a new patch be released without this? If so, do you know when? Haven't found a way to downgrade to 7.4.16 on Ubuntu.

mbeccati · 2021-05-03T07:48:27Z

@tbleckert The bug fix has been reverted on git and the revert will be in next point releases, that's all I know 🤷

nikic · 2021-05-03T07:52:58Z

@tbleckert Assuming you are using Ondrej's PPA, you might be able to request a backport of the revert at https://github.com/oerdnj/deb.sury.org. Not sure what their policy on this is.

nikic · 2021-05-03T08:33:41Z

https://bugs.php.net/bug.php?id=80892#1620030664 indicates that this change probably isn't viable even for 8.1. The pgsql type system is too strict for this to work.

mbeccati · 2021-05-03T08:43:05Z

@nikic Yes, it is possible that we can't fix the original issue without introducing other problems. That sucks, but I'm more and more inclined to drop all of this to avoid wasting time doing pointless research for 8.1.

mvorisek · 2021-05-03T08:46:21Z

In atk4/dsql we run tests across all DB vendors and fix was needed, see https://github.com/atk4/dsql/pull/299/files#diff-cac2b61f4ef042b487661e448d66b69392b1de2f82c4a0e1de1a2fd1be614bffR532, however this change is welcomed from my side in PHP 8.1.

I also find out that Oracle does not support boolean type at at, but php oci driver does not convert PARAM_BOOL to PARAM_INT. This is why many packages converts bool types to int. So I think we need to fix support of PARAM_BOOL across all DB drivers first and then we can merge this PR (into next minor release).

nikic · 2021-05-03T10:59:03Z

Looks like https://bugs.php.net/bug.php?id=81006 is people hitting the segfault part of this issue.

hansgv · 2021-05-03T14:37:08Z

@tbleckert Assuming you are using Ondrej's PPA, you might be able to request a backport of the revert at https://github.com/oerdnj/deb.sury.org. Not sure what their policy on this is.

It would appear Ondrej has released 7.4.18-2. Anyone know if this issue was fixed?

driesvints · 2021-05-03T14:45:32Z

@hansgv seems like 8.0.5-2 at least solved it: laravel/framework#37215 (comment)

yalsicor · 2021-05-03T14:45:57Z

@hansgv Ondrej reverted the PR that was causing the pgsql package to break installations (eg. laravel). That works as far as I am concerned.

The subject of this issue is another and does not seem to have a solution yet (and might not get one soon).

Gemorroj · 2021-05-03T16:52:12Z

for centos (remi repo):

yum downgrade php-* --exclude=php-pecl-*

NeilJ247 · 2021-05-06T10:17:24Z

I have had to set our docker image version php:7.4.16-fpm-alpine which "fixes" the is of type boolean but expression is of type integer error I was seeing yesterday.

nikic · 2021-05-06T20:59:44Z

For the record, PHP 7.4.19 / PHP 8.0.6 have been / will be released outside the usual schedule, with a fix for this regression only.

Fix bug #80892 PDO::PARAM_INT on pdo_pgsql

950efed

mbeccati closed this Apr 12, 2021

mbeccati deleted the bug80892 branch April 12, 2021 06:23

nikic reviewed Apr 28, 2021

View reviewed changes

mvorisek mentioned this pull request May 1, 2021

Fix PostgreSQL boolean handling for PHP 8.0.5 atk4/dsql#299

Merged

malohtie mentioned this pull request May 1, 2021

Unable to query for "true" in where() statement in Laravel query builder with PostgreSQL laravel/framework#37205

Closed

driesvints mentioned this pull request May 2, 2021

Registration fails at creating personal Team with PostgreSQL laravel/jetstream#778

Closed

yalsicor mentioned this pull request May 3, 2021

pgsql package 7.4.18 breaking database connection oerdnj/deb.sury.org#1598

Closed

1 task

timoschwarzer mentioned this pull request May 3, 2021

Internal Server Error when trying to edit or create entries craftcms/cms#7874

Closed

securit mentioned this pull request May 4, 2021

Postgresql SQLSTATE[42804]: Datatype mismatch: 7 column "active" is of type boolean but expression is of type integer laravel/framework#37247

Closed

Gemorroj referenced this pull request May 4, 2021

Update versions for PHP 7.4.19

52e517a

EtienneM mentioned this pull request May 5, 2021

Upgrade PHP support 8.0.3 to 8.0.7, 7.4.16 to 7.4.20, 7.3.26 to 7.3.28 Scalingo/php-buildpack#201

Closed

scottperlitch mentioned this pull request May 5, 2021

Upgrading 7.4.18 with patch included shivammathur/homebrew-php#307

Closed

pokgak mentioned this pull request May 20, 2021

Update/Add PHP to version 7.4.19 and 8.0.6 actions/runner-images#3426

Closed

7 tasks

bastien-phi mentioned this pull request Aug 11, 2023

Improper treatment of booleans in QueryBuilder::toRawSql for Postgres laravel/framework#48032

Closed

Fix bug #80892 PDO::PARAM_INT on pdo_pgsql #6801

Fix bug #80892 PDO::PARAM_INT on pdo_pgsql #6801

Uh oh!

Conversation

mbeccati commented Mar 23, 2021

Uh oh!

mbeccati commented Apr 12, 2021

Uh oh!

taylorotwell commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

mbeccati Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

mbeccati commented Apr 28, 2021

Uh oh!

kamil-tekiela commented Apr 28, 2021

Uh oh!

mbeccati commented Apr 29, 2021

Uh oh!

mbeccati commented Apr 29, 2021

Uh oh!

nikic commented Apr 29, 2021

Uh oh!

mbeccati commented Apr 29, 2021

Uh oh!

nikic commented Apr 29, 2021

Uh oh!

mbeccati commented Apr 29, 2021

Uh oh!

mvorisek commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hansgv commented May 2, 2021

Uh oh!

mbeccati commented May 3, 2021

Uh oh!

mbeccati commented May 3, 2021

Uh oh!

tbleckert commented May 3, 2021

Uh oh!

mbeccati commented May 3, 2021

Uh oh!

nikic commented May 3, 2021

Uh oh!

nikic commented May 3, 2021

Uh oh!

mbeccati commented May 3, 2021

Uh oh!

mvorisek commented May 3, 2021

Uh oh!

nikic commented May 3, 2021

Uh oh!

hansgv commented May 3, 2021

Uh oh!

driesvints commented May 3, 2021

Uh oh!

yalsicor commented May 3, 2021

Uh oh!

Gemorroj commented May 3, 2021

Uh oh!

NeilJ247 commented May 6, 2021

Uh oh!

nikic commented May 6, 2021

Uh oh!

Uh oh!

taylorotwell commented Apr 28, 2021 •

edited

Loading

mvorisek commented Apr 30, 2021 •

edited

Loading