Skip to content

Bug #62593 Updated pdo_pgsql driver to convert boolean values to pg nati... #198

Closed
wants to merge 7 commits into from

4 participants

@willfitch

...ve format in emulation mode

Change-Id: Id81a7ae42e4108c126abb29f927950fea754144c

@willfitch

From the internals list:

The following cases cause pgsql boolean types to be converted to an incompatible (long) format:

  1. PQprepare is not available (HAVE_PQPREPARE is undefined). This happens when the libpq version < 8.0
  2. PQprepare is available, but either PDO_PGSQL_ATTR_DISABLE_NATIVE_PREPARED_STATEMENT or PDO_ATTR_EMULATE_PREPARES are true (emulation handled by PDO, and the parameter hook pgsql_stmt_param_hook just skips parameter checks)

This results in PDO converting the parameter to a long (default behavior for boolean). Take the following example:

$pdo = new PDO($dsn);
$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, true);
$query = $pdo->prepare( 'SELECT :foo IS FALSE as val_is_false' );
$query->bindValue( ':foo', false, PDO::PARAM_BOOL );
$query->execute( );
print_r($query->errorInfo());

This results in the following:

Array
(
[0] => 42804
[1] => 7
[2] => ERROR: argument of IS FALSE must be type boolean, not type integer at character 8
)

This happens because true and false are converted to their long formats (1 and 0 respectively), which are invalid values for Postgres. However, in the sole event that PQprepare is available and emulation is disabled, boolean parameters are correctly converted to "t" and "f".

As noted in bug #62593, disabling emulation fixes the issue. There are a couple of issues with this approach, though. First, it forces you to make multiple calls to the server when you actually only need to escape input. Second, and most important in my case, when using middleware like pgbouncer, it's not possible to use true prepared statements. The calls from PQprepare and PQexec will have separate handles.

The attached patch updates the driver to behave like so:

  1. Do we have PQprepare and is emulation turned off? If so, let the driver handle via PQprepare and PQexec
  2. Is PQprepare unavailable? If so, modify the original param by replacing the long 1 or 0 format to "t" or "f"
  3. Is PQprepare available and emulation turned on? If so, modify the original param by replacing the long 1 or 0 format to "t" or "f"
@lstrojny

Thanks! We need a test for that.

@willfitch

There actually is one, and it currently fails: bug_33876.phpt. This actually makes that specific test pass.

willfitch added some commits Sep 20, 2012
@willfitch willfitch Bug #62593 Updated pdo_pgsql driver to convert boolean values to pg n…
…ative format in emulation mode

Change-Id: Id81a7ae42e4108c126abb29f927950fea754144c
0e7f739
@willfitch willfitch Bug #62593 Added test for change
Change-Id: I21ffe7e8913b367e447afca2b1b9079b0dcbfb70
eb599b4
@willfitch

I went ahead and added a new test for this bug. Some of the tests from #33876 aren't relevant to this and I'd like to confirm this specific test only.

@wez
wez commented Sep 25, 2012

for the sake of completeness, can you add something like this to the test case?

 $value = false;
 $query->bindParam(':foo', $value, PDO::PARAM_BOOL);
 print_r($value);

so that we can assert that we didn't change the value of $value?

@willfitch

@wez I had to update SEPARATE_ZVAL_IF_NOT_REF to SEPARATE_ZVAL; otherwise, the original value gets modified on bindParam. I've updated the test to verify bindValue and bindParam behave as expected.

@wez
wez commented Sep 26, 2012

Hmmm, ok, next possible wrinkle then; can you try something like Example 3 from the manual crossed with the above?
http://php.net/manual/en/pdostatement.bindparam.php
but where the parameter is a bool?
You'll need to define a procedure with an OUT parameter; in this case we expect $value to get modified; want to make sure we didn't break that :-)

@willfitch

Haha. Well, there are a couple of issues here. First, PDO_PARAM_INTPUT_OUTPUT doesn't work for most drivers (see https://bugs.php.net/bug.php?id=43887). I have updated to specifically skip conversion of the param_type is hashed with PDO_PARAM_INPUT_OUTPUT. This event could cause a memory leak otherwise, so this is good. This does not, however, address the other bug and probably shouldn't.

I did not add the INOUT test as it will fail regardless.

@willfitch

@wez any other issues you can think of? I'm going to commit the test with the example 3 code but commented out. When we are able to address the other bug, the comments can be removed.

@willfitch

@wez @lstrojny If there are no further issues with this, could we get this merged? It is a show-stopper for those of us attempting to upgrade from 5.2 to 5.3/5.4.

@wez
wez commented Oct 3, 2012

The code looks good to me. I'm a little out of touch with getting things merged these days, so will need to appeal to @lstrojny and others to get it merged.

Thanks!

@willfitch

Thanks, @wez. I appreciate your feedback

@willfitch

@lstrojny Can we get this merged/queued for merging?

@dsp
php.net member
dsp commented Oct 30, 2012

I rebased it and currently waiting for Johannes approval for 5.3.

@dsp
php.net member
dsp commented Oct 30, 2012

Merged

@dsp dsp closed this Oct 30, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.