The PDO driver for PostgreSQL is inserting boolean FALSE values in to numeric columns as an empty string which causes PostgreSQL errors, its a bigger battle to make developers actually typecast values correctly - I blame PHP for being so lenient towards it.
Until this bug is fixed in the PDO driver I think this patch needs to be implied to ensure no boolean values are passed as parameters to the PostgreSQL Driver in Drupal.

Thanks to Damian for the speed patch suggestion.

CommentFileSizeAuthor
#14 467474-typecasting-gone-wrong.2.patch1.8 KBjosh waihi
#3 467474-typecasting-gone-wrong.patch1.75 KBjosh waihi
postgres-work-around.patch858 bytesjosh waihi

Comments

Crell’s picture

Crell commented

Can we see a benchmark on this? We're talking about iterating over every parameter in every query we run; that's a lot of loops, and I don't want to slow down the DB unnecessarily.

josh waihi’s picture

josh waihi commented

@Crell, I'm open to other suggestions - I've talked with DamZ about this, the only other option I can see is making Drupal developers type cast correctly which will be an ongoing maintenance factor for both core and contrib. That not something, as a driver maintainer, I want to do - this is something that should be dealt with in the driver, and I see it as a temporary solution till it gets fixed in the PDO driver itself.

On a performance side note, I managed to optimize postgres so tests now take 2 1/2 hours instead of 4 hours +.

josh waihi’s picture

josh waihi commented
StatusFileSize
new 467474-typecasting-gone-wrong.patch1.75 KB

The same issue occurred in the filter module, so I'm attaching a patch that fixes the query. However, one way around this would be to configure the mysql driver to act like the postgres driver on this one - this would make developers more aware and better programmers who typecast there values correctly?

Crell’s picture

Crell commented

There's another issue in the queue somewhere to auto-cast booleans for MySQL, since it breaks there, too. Again, I'm worried about the performance implications so we really need benchmarks to know how badly we're shooting ourselves in the foot.

damien tournoud’s picture

damien tournoud commented

Part of the answer: on my development VM (a VMWare inside my MacBook), running this loop on an array of 100 strings (in which no replacement needs to be made) consistently takes 55 us. For comparison purposes, running a straightforward prebuilt query with 100 parameters takes about 550 us, so it seems that we are talking of a loss of about 10% here.

Here is the script I used:


define('DRUPAL_ROOT', getcwd());

require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

$count = 100;
$repetitions = 10000;

$paths = array();
for ($j = 0; $j < $count; $j++) {
  $paths[':path' . $j] = 'node/' . $j;
}

timer_start('timer');
for ($i = 0; $i < $repetitions; $i++) {
  foreach ($paths as &$path) {
    if (is_bool($path)) {
      $path = (int) $path;
    }
  }
}
$total = timer_stop('timer');
print $total['time'] / $repetitions . "\n";

$conditions = array();
for ($j = 0; $j < $count; $j++) {
  $conditions[] = 'path = :path' . $j;
}
$query = "SELECT path FROM {menu_router} WHERE " . implode(' OR ', $conditions);

timer_start('timer');
for ($i = 0; $i < $repetitions; $i++) {
  db_query($query, $paths);
}
$total = timer_stop('timer');
print $total['time'] / $repetitions . "\n";
damien tournoud’s picture

damien tournoud commented
Title: Type casting boolean values as integers for PostgreSSQL Driver » Type casting boolean values as integers for PostgreSQL Driver
Crell’s picture

Crell commented

Wait, so you mean this patch has a 10% performance hit?

damien tournoud’s picture

damien tournoud commented

Crell: on 100 arguments yes.

Here is roughly how this scale:

Number of arguments // Cost of the replacement // Cost of the query
1 // 0.0008 // 0.132
10 // 0.0054 // 0.1707
100 // 0.0569 // 0.6243
1000 // 0.5534 // 4.2483

For 1 argument, the overhead is < 1%, for 10 arguments, it is in the 3-4% range, for 100 arguments it reaches about 10%, and a little bit more then 10% for 1000 arguments.

Crell’s picture

Crell commented

Can we get a benchmark on a Drupal page load rather than a microbenchmark? That should give us an idea of how much we're actually going to lose here.

damien tournoud’s picture

damien tournoud commented

@Crell: given the microbenchmarks, I don't believe the difference is even measurable on a full page load.

josh waihi’s picture

josh waihi commented

For Crell, I bootstrapped Drupal then ran this:

  $start = time();
  global $user;
  $user = user_load(array('uid' => 1));

  for ($i=0; $i < 100; $i ++) {
    menu_execute_active_handler('admin/build/modules');
  }

  $stop = time();
  echo 'Total time: ' . ($stop - $start) . 's' . "\n";

With out the patch: 80s
With the patch:87s

so thats 0.07 seconds difference for a page load of the modules page.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Damz just asked me to benchmark this. I'm not going to.

Let's not hold up something that's completely broken on benchmarks - get it actually working, then get a postgres test slave, then worry about optimization when that's done.

So many patches get in without any benchmarks at all, and slow Drupal down, we know the impact is minimal, that's enough information.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Can someone explain to me why it's a good idea to make pgsql auto-typecast and not mysql/sqlite as well?

josh waihi’s picture

josh waihi commented
StatusFileSize
new 467474-typecasting-gone-wrong.2.patch1.8 KB

The reason we're typecasting in PostgreSQL is because of a bug in the PDO Driver that doesn't handle binding parameters with associative arrays very well. For example:

try {

      $dbh = new PDO('pgsql:dbname=test;', 'myUser');
      $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
      $dbh->query('CREATE TABLE test (id int not null, name text not null)');

      $SQL = 'INSERT INTO test (id,name) VALUES (:id, :name)';
      $query = $dbh->prepare($SQL);
      $query->execute(array(':id' => 0, ':name' => 'This will work')); 
      $query->execute(array(FALSE, 'This will work'));
      $query->execute(array(':id' => FALSE, ':name' => 'This will not work'));

    } catch (Exception $e) {
      print $e->getMessage();
    }

    $dbh->query('DROP TABLE test');

Notice the second execute call will work but the third will not. Its the way in which we bind parameters that breaks this driver. If we were to bind parameters numerically and in order, ie, not use associative arrays, then this issue would not be here.

I've commented the code as per webchick's request.

snow-2’s picture

snow-2 commented

I'm not convinced it's a problem with pdo_pgsql. It actually specifically has code in there to deal with boolean types (though it changes them to 't' and 'f' and not '1' and '0' which would be better in this case). Here's what I've seen under gdb:

(gdb) p *param
$37 = {paramno = 0, name = 0x10dec80 ":id", namelen = 3, max_value_len = 0, parameter = 0x10deb10, param_type = PDO_PARAM_STR, driver_params = 0x0,
driver_data = 0x0, stmt = 0x10dd398, is_param = 1}
(gdb) p *param->parameter
$38 = {value = {lval = 17684240, dval = 8.7371754568116058e-317, str = {val = 0x10dd710 "", len = 0}, ht = 0x10dd710, obj = {handle = 17684240,
handlers = 0x0}}, refcount = 1, type = 6 '\006', is_ref = 0 '\0'}

This appears to indicate that, for whatever reason, PHP/PDO thinks this is an empty string, not a false boolean. I'm not sure yet why it works with the regular parameters, but that's next.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Hm. Well. Given that:

a) Any performance implications (which seem to be pretty minor according to #11) incurred by this patch only affect pgsql (for now)
b) It's an upstream bug that we can't do anything about
c) It enables us to get tests passing for pgsql which can in turn lead to a more robust test framework

I've committed this to HEAD.

I have a feeling we'll be revisiting this before D7 is released. :P

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.