PDO PostreSQL - parser is broken for `'...\'`

mvorisek commented 5 months ago

Description

The following code:

<?php

$pdo = new PDO('pgsql:host=127.0.0.1;dbname=xxx', 'user', 'pass');

$sql = <<<'EOF'
    select :a x, '\' x2
    EOF;
$statement = $pdo->prepare($sql);
$statement->bindValue('a', 'va');

$statement->execute();
$res = $statement->fetchAll(PDO::FETCH_ASSOC);
var_dump($res);

$sql = <<<'EOF'
    select :a x, '\' x2, :b y, '\' y2
    EOF;
$statement = $pdo->prepare($sql);
$statement->bindValue('a', 'va');
$statement->bindValue('b', 'vb');

$statement->execute();
$res = $statement->fetchAll(PDO::FETCH_ASSOC);
var_dump($res);

Resulted in this output:

array(1) {
  [0]=>
  array(2) {
    ["x"]=>
    string(2) "va"
    ["x2"]=>
    string(1) "\"
  }
}

Warning: PDOStatement::bindValue(): SQLSTATE[HY093]: Invalid parameter number: :b in C:\...\repro.php on line 20
array(0) {
}

But I expected this output instead:

array(1) {
  [0]=>
  array(2) {
    ["x"]=>
    string(2) "va"
    ["x2"]=>
    string(1) "\"
  }
}
array(1) {
  [0]=>
  array(4) {
    ["x"]=>
    string(2) "va"
    ["x2"]=>
    string(1) "\"
    ["y"]=>
    string(2) "vb"
    ["y2"]=>
    string(1) "\"
  }
}

'\' is causing the issue - but it is absolutely correct string syntax.

Identifier escaping ("\") is broken as well.

It seems php-src parses the SQL using the old mode (default until PostgreSQL 9.1) - https://www.postgresql.org/docs/current/runtime-config-compatible.html#GUC-STANDARD-CONFORMING-STRINGS.

PHP Version

any (tested 7.4 and 8.3)

Operating System

any (tested Windows and linux)

mvorisek commented 5 months ago

@devnexen this is definitely a bug, the SQL is parsed somehow by php wrongly.

devnexen commented 5 months ago

the extension does not support it (AFAIK part of this extension still rely on old postgresql versions behavior), I do not think it qualifies for php 8.2/8.3 though.

mvorisek commented 5 months ago

The relation with PostgreSQL 9.1 is my pure guess, but if true, the version was released more than 12 years ago. So I would say we have a bug in SQL parsing affecting basically all pdo pgsql users.

MorganLOCode commented 5 months ago

Note in contrast that the PostgreSQL extension does parse the string literal correctly:

$connection = pg_connect('host=127.0.0.1 dbname=xxx user=xxx password=xxx');

$sql = <<<'EOF'
    select $1 x, '\' x2
    EOF;
$statement = pg_prepare($connection, 'q1', $sql);
$result = pg_fetch_all(pg_execute($connection, 'q1', ['va']));

var_dump($result);

$sql = <<<'EOF'
    select $1 x, '\' x2, $2 y, '\' y2
    EOF;
$statement = pg_prepare($connection, 'q2', $sql);
$result = pg_fetch_all(pg_execute($connection, 'q2', ['va', 'vb']));

var_dump($result);

devnexen commented 5 months ago

pdo pgsql and pgsql are distinct code wise :)

mvorisek commented 5 months ago

It seems the parsing is called in https://github.com/php/php-src/blob/php-8.3.0/ext/pdo_pgsql/pgsql_driver.c#L292 and done in https://github.com/php/php-src/blob/php-8.3.0/ext/pdo/pdo_sql_parser.re#L58 where it assumes MySQL behaviour instead of ANSI.

here are two correct examples of how it should be done:

doctrine/dbal - https://github.com/doctrine/dbal/blob/4.0.1/src/SQL/Parser.php#L60
atk4/data - https://github.com/atk4/data/blob/b0238b1d72/src/Persistence/Sql/Expression.php#L36 (ANSI) and https://github.com/atk4/data/blob/b0238b1d72/src/Persistence/Sql/Mysql/Expression.php#L15 (MySQL)

MorganLOCode commented 5 months ago

pdo pgsql and pgsql are distinct code wise :)

Yes, I know. One works correctly (it defers to libpq), the other doesn't.

SakiTakamachi commented 5 months ago

After some testing, I think this might qualify as a bug.

However, I think this is a bit of a gray area, right on the border between a bug and a feature request.

As mentioned, this is due to PDO's common query parser not taking into account PostgreSQL-specific behavior.

I'm not sure how much change will be needed until I work a bit more into this, but I'll get started.

MorganLOCode commented 5 months ago

Well, as far as quoting goes, recognising '\' as a string literal containing a single backslash isn't so much PostgreSQL-specific as standard ISO/ANSI behaviour.

mvorisek commented 5 months ago

Thank you for looking into this.

Here is big testcase string: https://github.com/atk4/data/blob/c2baf07324/tests/Persistence/Sql/WithDb/SelectTest.php#L552-L582

and testcase for identifiers: https://github.com/atk4/data/blob/c2baf07324/tests/Persistence/Sql/WithDb/SelectTest.php#L622-L648 (plus all cases prefixed /w \\ - https://github.com/atk4/data/blob/c2baf07324/tests/Persistence/Sql/WithDb/SelectTest.php#L675)