When working with PostgreSQL, it will always buggy when input an empty string '' into a BLOB field though db_insert() or db_update(). Apache will even killed by Segmentation fault (11).

Reproduce procedure:
1. Apply patch attached. This will add 2 set of simpletest for INSERT/UPDATE an empty string into DB.
2. Activate simpletest.
3. Run simpletest with "Database => Insert tests, LOB fields" and "Database => Update tests, LOB".

Expected result:
Test should pass for both MySQL and PostgreSQL.

Final result:
MySQL: pass.
PostgreSQL: Can't INSERT/UPDATE an empty string into DB, and even generate error message with [notice] child pid 11193 exit signal Segmentation fault (11).

Any idea for solving this problem? Or what should be the best way to work with PostgreSQL's BLOB field?

Files: 
CommentFileSizeAuthor
#15 DRUPAL_MINIMUM_PHP-1223990404.patch650 byteshswong3i
Failed: Failed to install HEAD.
[ View ]
#15 testEmptyBlob-1223990157.patch4.37 KBhswong3i
Failed: 7676 passes, 1 fail, 0 exceptions
[ View ]
#15 tngdb-pdo_pgsql-blob-1223989019.patch4 KBhswong3i
Unable to apply patch tngdb-pdo_pgsql-blob-1223989019.patch
[ View ]
#8 testEmptyBlob-1223437816.patch3.86 KBhswong3i
Failed: Failed to install HEAD.
[ View ]
testEmptyBlob-1222926977.patch2.2 KBhswong3i
Failed: Failed to install HEAD.
[ View ]

Comments

hswong3i’s picture

Title:[TNGDB + pgsql] db_insert/db_update buggy with empty string into BLOB» [TNGDB + pgsql] db_insert/db_update buggy with empty string input and BLOB field
hswong3i’s picture

Seems it is already mentioned in bugs.php.net (http://bugs.php.net/bug.php?id=41135), isn't it? But I am not sure if it can solve the case of PostgreSQL, too :S

hswong3i’s picture

Also test with php5.3-200810020430.tar.gz and still fail :S

Crell’s picture

Hm. Well that's irritating. I don't think the bug you link to is relevant, as that's talking about SQLite truncating a BLOB that does have data at the first null character; what you're talking about here is Postgres dropping the connection on an empty BLOB.

Can we confirm if this is a Postgres problem or a PDO problem, and if it's a known bug? What version of PHP and Postgres are you running? We can probably work around the problem by checking for empty values and then skipping over that field in the insert/update statement, but I want to confirm exactly what the problem is first so we can document it.

hswong3i’s picture

This bug should belongs to pdo_pgsql but not PostgreSQL itself. I simplify and clone the programming logic of our TNG DB abstraction and database_test.test as below.

Here are some code for reproduce the bug.

Prepare testing database and table

createdb -O postgres DRUPAL_7
su - postgres
psql DRUPAL_7

DRUPAL_7=# CREATE TABLE "test_one_blob" (
id SERIAL NOT NULL,
blob1 BYTEA
);

Testing with psql directly (CORRECT)

DRUPAL_7=# INSERT INTO "test_one_blob" ("blob1") VALUES (E'This is\\000a test.');
DRUPAL_7=# INSERT INTO "test_one_blob" ("blob1") VALUES ('');
DRUPAL_7=# INSERT INTO "test_one_blob" ("blob1") VALUES (NULL);

DRUPAL_7=# SELECT * FROM "test_one_blob";

Testing with PHP5.2 and pdo_pgsql (BUGGY!)

<?php
function db_insert($sql, $data) {
  global
$active_db;
 
$stmt = $active_db->prepare($sql);
 
$blob = fopen('php://memory', 'a');
 
fwrite($blob, $data);
 
rewind($blob);
 
$stmt->bindParam(':blob1', $blob, PDO::PARAM_LOB);
 
$stmt->execute();
 
$id = $active_db->lastInsertId('test_one_blob_id_seq');
  return
$id;
}

function
db_select($sql, $id) {
  global
$active_db;
 
$sql = 'SELECT * FROM "test_one_blob" WHERE "id" = :id';
 
$stmt = $active_db->prepare($sql);
 
$stmt->bindParam(':id', $id);
 
$stmt->execute();
  return
$stmt->fetch(PDO::FETCH_ASSOC);
}

$active_db = new PDO('pgsql:host=localhost dbname=DRUPAL_7', 'root', 'CHANGE', array(PDO::ATTR_STRINGIFY_FETCHES => TRUE));

// Test normal BLOB insert
$id = db_insert('INSERT INTO "test_one_blob" ("blob1") VALUES (:blob1)', "This is\000a test.");
$return = db_select('SELECT * FROM "test_one_blob" WHERE "id" = :id', $id);
var_dump($return['blob1']);

// Test NULL BLOB insert
$id = db_insert('INSERT INTO "test_one_blob" ("blob1") VALUES (:blob1)', NULL);
$return = db_select('SELECT * FROM "test_one_blob" WHERE "id" = :id', $id);
var_dump($return['blob1']);

// Test empty BLOB insert (BUGGY!!)
$id = db_insert('INSERT INTO "test_one_blob" ("blob1") VALUES (:blob1)', "");
$return = db_select('SELECT * FROM "test_one_blob" WHERE "id" = :id', $id);
var_dump($return['blob1']);
?>
hswong3i’s picture

Also report to bugs.php.net:
http://bugs.php.net/46249

Crell’s picture

Can you provide a patch that adds some unit tests to check for this behavior? It should be easy to add to the existing Insert-Blob test class. Then we can add a fix in the postgres driver and link to that PHP bug. Thanks!

hswong3i’s picture

Status:Active» Needs review
StatusFileSize
new3.86 KB
Failed: Failed to install HEAD.
[ View ]

Add INSERT/UPDATE BLOB test for both NULL and empty string.

Tested with MySQL and PostgreSQL, PHP5.2.6 and PHP5.3-dev. MySQL is safe as expected, where PostgreSQL also BUGGY with empty string input as expected.

P.S. After asking help from IRC #postgresql, people suggest we may set existing BLOB fields as "nullable" in order to escape from the trap. BTW, some programming logic may also need to update IF we are using BLOB and compare its content with empty string :S

Crell’s picture

Did the #postgresql folks say why it was better to solve this with a "nullable" in the schema than to skip an empty field entirley in our abstraction layer?

linuxpoet’s picture

Setting the field to nullable would allow you to keep the abstraction. I don't see any particular downside to it.

hswong3i’s picture

Because some tables are now coming with NOT NULL BLOB fields. When we INSERT/UPDATE empty string (which should be correct in programming logic) in these fields, pdo_pgsql will translate it as NULL and so report error.

I mention the above concern in #postgresql. aglio2 suggest that setting fields as "nullable" can be a quick solution, or else we may need to wait for months before pdo_pgsql get fixed.

P.S. I am also giving a try for http://drupal.org/node/147947#comment-1049609 related issues and most are functioning with "nullable" BLOB fields. Only face difficulty with node related changes and may need help soon :-)

Crell’s picture

OK, so for those of us who don't know from Postgres... is Nullable a schema change or a query change? :-)

hswong3i’s picture

We can handle nullable within *.install. Example: http://drupal.org/node/300802#comment-1049737

hswong3i’s picture

Followup: This issue is now figure out as PHP pdo and pdo_pgsql bugs (http://bugs.php.net/bug.php?id=46249). I am now actively communicate with felipe@php.net and hopefully it will soon get fixed within PHP CVS HEAD. It should coming with PHP5.2.7 (it is now 5.2.7RC2).

I will suggest for the commit of patch in #8, and let PostgreSQL's simpletest come with a bit warning at this moment. Soon after the pdo and pdo_pgsql get fixed, we can document the minimal PHP version requirement as 5.2.7. Any idea?

hswong3i’s picture

StatusFileSize
new4 KB
Unable to apply patch tngdb-pdo_pgsql-blob-1223989019.patch
[ View ]
new4.37 KB
Failed: 7676 passes, 1 fail, 0 exceptions
[ View ]
new650 bytes
Failed: Failed to install HEAD.
[ View ]

This bug is now get fixed in PHP 5.2 and 5.3 CVS HEAD. All packages newer than snapshot php5.2-200810130030.tar.gz. is safe from this issue. Please check http://bugs.php.net/bug.php?id=46249 for more detail.

Patch files detail:

  • tngdb-pdo_pgsql-blob-*.patch: BLOB NULL is a bit different from normal case: we can't use StreamAPI, but bind NULL to target field direcly. Because writing NULL into file handler means write nothing. The file handler will contain an empty string but not NULL. Therefore NULL will be translated as empty string when save into database, and so buggy. This patch fix the above issue.
  • testEmptyBlob-*.patch: Similar as case before, add simpletest test case for both NULL and empty BLOB INSERT/UPDATE. Also update comment message and external links as bug reference.
  • DRUPAL_MINIMUM_PHP-*.patch: Since it is just get fixed in PHP CVS HEAD, we will need to push our minimal PHP support version to coming 5.2.7 (as it is now 5.2.7RC2-dev). This can postpone till our D7 public release, or the official release of PHP 5.2.7.

All patches tested with MySQL and PostgreSQL, and all passed. It is a critical issue for PostgreSQL support and so may someone give a hand for it?

hswong3i’s picture

P.S. I wrote some note for install Apache2.2 + PHP5.2 + PostgreSQL manually, and you may find this as useful for setting up your own testbed: Apache2.2 + PHP5.2 + pgsql/pdo_pgsql from sketch on Debian sid HOWTO

hswong3i’s picture

Title:[TNGDB + pgsql] db_insert/db_update buggy with empty string input and BLOB field» [DBTNG + pgsql] db_insert/db_update buggy with empty string input and BLOB field
Crell’s picture

I don't think we can reasonably require PHP 5.2.7, unfortunately. It may be different when D7 actually ships, for now let's assume we can't do that and code accordingly.

hswong3i’s picture

@Crell: Totally agree with you, and so the DRUPAL_MINIMUM_PHP-*.patch should postpone until suitable timing.

BTW, the tngdb-pdo_pgsql-blob-*.patch is the correct handling of BLOB field if database required for variable binding. I use similar code snippet when develop Oracle and DB2 drivers. This patch should able to handle independently.

For the testEmptyBlob-*.patch, it is already well documented that it won't pass until we are using PHP version newer than snapshot php5.2-200810130030.tar.gz, so there shouldn't be any confuse. Maybe just let the test case result with warning, when we are using PHP with old version?

hswong3i’s picture

Latest complete version of bug reproduction code snippet:

<?php
function db_insert($sql, $data) {
  global
$active_db;
 
$stmt = $active_db->prepare($sql);
  if (
is_null($data)) {
   
$stmt->bindParam(':blob1', $data, PDO::PARAM_LOB);
  }
  else {
   
$blob = fopen('php://memory', 'a');
   
fwrite($blob, $data);
   
rewind($blob);
   
$stmt->bindParam(':blob1', $blob, PDO::PARAM_LOB);
  }
 
$stmt->execute();
 
$id = $active_db->lastInsertId('test_one_blob_id_seq');
  return
$id;
}

function
db_select($sql, $id) {
  global
$active_db;
 
$sql = 'SELECT * FROM "test_one_blob" WHERE "id" = :id';
 
$stmt = $active_db->prepare($sql);
 
$stmt->bindParam(':id', $id);
 
$stmt->execute();
  return
$stmt->fetch(PDO::FETCH_ASSOC);
}

$active_db = new PDO('pgsql:host=localhost dbname=DRUPAL_7', 'root',
'CHANGE', array(PDO::ATTR_STRINGIFY_FETCHES => TRUE));

// Test normal BLOB insert
$data = "This is\000a test.";
$id = db_insert('INSERT INTO "test_one_blob" ("blob1") VALUES (:blob1)',
$data);
$return = db_select('SELECT * FROM "test_one_blob" WHERE "id" = :id',
$id);
var_dump("Test normal BLOB insert");
var_dump($data);
var_dump($return['blob1']);
var_dump($data === $return['blob1']);

// Test NULL BLOB insert
$data = NULL;
$id = db_insert('INSERT INTO "test_one_blob" ("blob1") VALUES (:blob1)',
$data);
$return = db_select('SELECT * FROM "test_one_blob" WHERE "id" = :id',
$id);
var_dump("Test NULL BLOB insert");
var_dump($data);
var_dump($return['blob1']);
var_dump($data === $return['blob1']);

// Test empty BLOB insert
$data = "";
$id = db_insert('INSERT INTO "test_one_blob" ("blob1") VALUES (:blob1)',
$data);
$return = db_select('SELECT * FROM "test_one_blob" WHERE "id" = :id',
$id);
var_dump("Test empty BLOB insert");
var_dump($data);
var_dump($return['blob1']);
var_dump($data === $return['blob1']);
?>
Dries’s picture

Crell, given that this patch only affects PostgreSQL it is probably acceptable to commit it now, and to worry about bumping the PHP version a couple of months from now? I'm still on 5.2.5 but hopefully all major Linux distributions will soon upgrade their PHP libraries.

Crell’s picture

I need to apply and test the other patches before I comment on them.

Re distros, Debian and Ubuntu are about to have their next stable releases in the next few weeks. Both will have at best 5.2.6, and be stuck on that for months or years. (5.2.7 isn't out yet.) We can and should require PHP 5.2.x, but I don't think we can get away with requiring the absolute latest minor release. I'm not sure at the moment what the best 5.2.x to target will be.

Crell’s picture

I'll also note that we do have the option of making 5.2.7 required just for Postgres, but not for Drupal in general. The DB driver can use whatever logic it wants to define whether or not it is installable, so there's no reason all DB drivers have to have the same PHP version requirements.

I'm not saying it's a good idea to do that, mind you, and if we can work around the problem without doing so I'd prefer it, but that is a possible option.

Status:Needs review» Needs work

The last submitted patch failed testing.

Crell’s picture

Status:Needs work» Needs review

Probably caused by the recent bot bug.

catch’s picture

Status:Needs review» Needs work

The test and the patch should be one file.

mgifford’s picture

I hate Segmentation faults. Just wanted to ping this to bring this issue to the top of the pile for D7. Also interested in back-porting to D6.

Josh Waihi’s picture

there is another fix in PHP 5.2.12 that would help the pgsql driver also regarding #515310: Inserting >PHP_INT_MAX into BIGINT fails on PostgreSQL. Since we have the installer that checks database conditions, we could issue a warning to postgresql users that PHP 5.2.12, PHP 5.3 or greater is recommended?

Crell’s picture

I'm pretty sure we can do version-specific DB drivers now, but 5.2.12 is extremely new. I don't think it's even out yet. I'm wary about requiring a version THAT new for Postgres. 5.2.3 or 5.2.4 to avoid bugs in early 5.2.x releases, sure, but those are easy to come by. No one is running 5.2.12 yet, and 5.3 is still an iffy .0 release.

Josh Waihi’s picture

XAMPP is already shipping 5.3 - but they're always jumping the gun. Does sound like PostgreSQL will have to roll on 5.3 because Drupal has really shown just how bad of a condition PDO is for DB other than MySQL.

Crell’s picture

Component:database system» postgresql database

This seems postgres specific...

sun.core’s picture

Hm... at least some linux distros ship with PHP 5.3 by default now.

catch’s picture

Title:[DBTNG + pgsql] db_insert/db_update buggy with empty string input and BLOB field» Raise PHP requirement to 5.2.12 for PostgreSQL only

5.2.12 was released mid-December 2009 - that's three and a half months ago now. I don't think we'll see 7.0 for another two months at least, so we can reasonably expect the first production D7 sites running postgres to be launched around six months after 5.2.12 was out.
Given that, I think making it a driver-specific requirement is pretty reasonable - sites running postgres are going to have more control over their server than most MySQL sites so it's a lot less about managing on crappy hosting providers - if you're not picky about your hosting then you're surely not picky enough to be using postgres ;)

catch’s picture

Priority:Critical» Normal

I'm also downgrading this from critical since the fix is simply to run PHP 5.2.12+ which can be documented in http://drupal.org/requirements, the requirements check on install is an additional nicety.

Crell’s picture

Assigned:hswong3i» Unassigned

I guess good things come to those who wait... :-) If Josh and Damien are OK with a 5.2.12 requirement for Postgres, I'm OK with it.

Josh Waihi’s picture

#515310: Inserting >PHP_INT_MAX into BIGINT fails on PostgreSQL adds a check at install and warns users running PostgreSQL and PHP < 5.2.12 that there is a problem and that they should upgrade but doesn't refuse to install. This is because, while PHP may have fixed the bug, that doesn't mean its reached the OS repositories such as Debian, Fedora, etc. While it is true that PostgreSQL users are more likely to have control over there environment, it doesn't mean that they wouldn't prefer to use what there OS of choice wants to support.

IMO opinion I am happy to warn users and then leave it up to them rather than make it a requirement. At least we can say "Well I warned you...." ;)

This issue seems like a duplicate of #515310: Inserting >PHP_INT_MAX into BIGINT fails on PostgreSQL. Thoughts?

Josh Waihi’s picture

Debian Squeeze will run 5.3.1+ and Ubuntu Lucid will run 5.3.1+ everything else below in ubuntu/debian land will not support PHP 5.2.12. I'm happier to signal to users a warning, unless this particular issue is fatal to PostgreSQL's use with Drupal. In which case we have no choice but to bump the version.

catch’s picture

Status:Needs work» Closed (duplicate)

If we have the warning on another issue, let's close this as a dup.