Bug #70953 for DBD-Pg: Named :placeholders error out depending on their order of appearance in a query

Wed Sep 14 01:23:11 2011 y [...] fedevych.name - Ticket created

Subject:

Named :placeholders error out depending on their order of appearance in a query

Using perl-5.12.3, DBD::Pg 2.18.1 on Debian GNU/Linux. There are two queries in the attached example file: UPDATE testtbl SET status = :status, status_info = :status_info WHERE id = 1; UPDATE testtbl SET status_info = :status_info, status = :status WHERE id = 1; One might say that they do the same thing. I also thought so. In reality, the first one executes while the second one fails with: Cannot bind unknown placeholder ':status' I think this must be a bug in DBD::Pg.

Subject:

test.pl

#!/usr/bin/env perl use strict; use warnings; use DBI; my $failing_query = "UPDATE testtbl SET status_info = :status_info, status = :status WHERE (id = :id)"; my $successful_query = "UPDATE testtbl SET status = :status, status_info = :status_info WHERE (id = :id)"; my $params = { ':status_info' => undef, ':status' => 'PROCESSED', ':id' => '1' }; my $dbh = DBI->connect( 'DBI:Pg:host=localhost;dbname=test', 'test', 'test' ); # Insert your own my $sth = $dbh->prepare($successful_query); foreach my $key ( keys %{$params} ) { $sth->bind_param( $key, $params->{$key} ); } $sth->execute(); print "This statement was executed:\n\n\t$successful_query\n\nThis now will fail:\n\n\t$failing_query\n\n"; $sth = $dbh->prepare($failing_query); foreach my $key ( keys %{$params} ) { $sth->bind_param( $key, $params->{$key} ); } $sth->execute(); 1; __END__ Table structure: CREATE TABLE testtbl ( id serial, status_info text, status character varying (32), primary key(id) ); INSERT INTO testtbl (id, status_info, status) VALUES (1, '', 'UNKNOWN');

Wed Nov 02 09:52:26 2011 JANPAZ [...] cpan.org - Correspondence added 30 min

Subject:

[PATCH] Named :placeholders error out depending on their order of appearance in a query

On Wed Sep 14 01:23:11 2011, jafd wrote: Show quoted text

> > One might say that they do the same thing. I also thought so. > In reality, the first one executes while the second one fails with: > > Cannot bind unknown placeholder ':status' > > I think this must be a bug in DBD::Pg.

Indeed. When parsing the placeholders, the code does strncmp which matches left prefix of previously found placeholders. Attached are patches against 1.49 (where I experienced the problem) and against 2.99 (where the code is still the same), with test updates. Please apply. Jan

Subject:

bind-params-2.99.patch

diff -ru DBD-Pg-2.99.9_2-orig/dbdimp.c DBD-Pg-2.99.9_2/dbdimp.c --- DBD-Pg-2.99.9_2-orig/dbdimp.c 2011-06-20 16:37:15.000000000 -0400 +++ DBD-Pg-2.99.9_2/dbdimp.c 2011-11-02 09:45:26.000000000 -0400 @@ -1944,7 +1944,8 @@ sectionsize = currpos-sectionstop; /* Have we seen this placeholder yet? */ for (xint=1,thisph=imp_sth->ph; NULL != thisph; thisph=thisph->nextph,xint++) { - if (0==strncmp(thisph->fooname, statement-sectionsize, sectionsize)) { + if (0==strncmp(thisph->fooname, statement-sectionsize, sectionsize) + && thisph->fooname[sectionsize] == '\0') { newseg->placeholder = xint; newseg->ph = thisph; break; diff -ru DBD-Pg-2.99.9_2-orig/t/02attribs.t DBD-Pg-2.99.9_2/t/02attribs.t --- DBD-Pg-2.99.9_2-orig/t/02attribs.t 2011-06-14 00:12:52.000000000 -0400 +++ DBD-Pg-2.99.9_2/t/02attribs.t 2011-11-02 09:43:19.000000000 -0400 @@ -18,7 +18,7 @@ if (! $dbh) { plan skip_all => 'Connection to database failed, cannot continue testing'; } -plan tests => 249; +plan tests => 250; isnt ($dbh, undef, 'Connect to database for handle attributes testing'); @@ -482,6 +482,10 @@ $sth = $dbh->prepare('SELECT 123 FROM pg_class WHERE relname=? AND reltuples=? and relpages=?'); is ($sth->{'NUM_OF_PARAMS'}, 3, $t); +$t='Statement handle attribute "NUM_OF_PARAMS" works correctly with placeholders in reverse order'; +$sth = $dbh->prepare("select :p10 + :p1"); +is ($sth->{'NUM_OF_PARAMS'}, 2, $t); + $t='Statement handle attribute "NUM_OF_PARAMS" works correctly before execute with one placeholder'; $sth = $dbh->prepare('SELECT 123 AS "Sheep", CAST(id AS float) FROM dbd_pg_test WHERE id=?'); is ($sth->{'NUM_OF_PARAMS'}, 1, $t);

Subject:

bind-params-1.49.patch

diff -ur DBD-Pg-1.49-orig/dbdimp.c DBD-Pg-1.49/dbdimp.c --- DBD-Pg-1.49-orig/dbdimp.c 2006-05-03 22:11:14.000000000 -0400 +++ DBD-Pg-1.49/dbdimp.c 2011-11-02 09:38:28.000000000 -0400 @@ -1311,7 +1311,8 @@ sectionsize = currpos-sectionstop; /* Have we seen this placeholder yet? */ for (xint=1,thisph=imp_sth->ph; NULL != thisph; thisph=thisph->nextph,xint++) { - if (0==strncmp(thisph->fooname, statement-sectionsize, sectionsize)) { + if (0==strncmp(thisph->fooname, statement-sectionsize, sectionsize) + && thisph->fooname[sectionsize] == '\0') { newseg->placeholder = xint; newseg->ph = thisph; break; diff -ur DBD-Pg-1.49-orig/t/02attribs.t DBD-Pg-1.49/t/02attribs.t --- DBD-Pg-1.49-orig/t/02attribs.t 2011-11-02 09:37:04.000000000 -0400 +++ DBD-Pg-1.49/t/02attribs.t 2011-11-02 09:37:30.000000000 -0400 @@ -8,7 +8,7 @@ $|=1; if (defined $ENV{DBI_DSN}) { - plan tests => 132; + plan tests => 133; } else { plan skip_all => 'Cannot run test unless DBI_DSN is defined. See the README file'; @@ -445,6 +445,11 @@ $sth->finish(); +$sth = $dbh->prepare("select :p10 + :p1"); +$attrib = $sth->{'NUM_OF_PARAMS'}; +is( $attrib, '2', 'Statement handle attribute "NUM_OF_PARAMS" works correctly with two placeholders'); +$sth->finish(); + # # Test of the statement handle attribute "CursorName" #

Wed Nov 02 09:52:28 2011 The RT System itself - Status changed from 'new' to 'open'

Wed Nov 02 10:02:03 2011 JANPAZ [...] cpan.org - Severity Normal added

Wed Nov 02 10:02:03 2011 JANPAZ [...] cpan.org - Broken in 1.49 added

Wed Nov 02 10:02:40 2011 JANPAZ [...] cpan.org - Broken in 2.99.9_2 added

Wed Nov 02 10:12:41 2011 JANPAZ [...] cpan.org - Correspondence added

Another possible patch for dbdimp.c is to just + 1 the tested length: diff -ru DBD-Pg-2.99.9_2-orig/dbdimp.c DBD-Pg-2.99.9_2/dbdimp.c --- DBD-Pg-2.99.9_2-orig/dbdimp.c 2011-06-20 16:37:15.000000000 -0400 +++ DBD-Pg-2.99.9_2/dbdimp.c 2011-11-02 10:10:59.000000000 -0400 @@ -1944,7 +1944,7 @@ sectionsize = currpos-sectionstop; /* Have we seen this placeholder yet? */ for (xint=1,thisph=imp_sth->ph; NULL != thisph; thisph=thisph->nextph,xint++) { - if (0==strncmp(thisph->fooname, statement-sectionsize, sectionsize)) { + if (0==strncmp(thisph->fooname, statement-sectionsize, sectionsize + 1)) { newseg->placeholder = xint; newseg->ph = thisph; break;

Fri Mar 09 11:25:02 2012 greg [...] turnstep.com - Correspondence added

From:

greg [...] turnstep.com

Thanks, patch applied in 7baf5cfe0c56a739a1a662d78221104622ee2bb8

Tue Aug 21 13:38:44 2012 greg [...] turnstep.com - Severity Normal changed to Important

Tue Aug 21 13:38:44 2012 greg [...] turnstep.com - Fixed in 2.19.0 added

Tue Aug 21 13:38:44 2012 greg [...] turnstep.com - Status changed from 'open' to 'resolved'