Bug #95173 for DBD-Pg: feature request: $ placeholders but not : placeholders

Tue Apr 29 13:18:49 2014 PLICEASE [...] cpan.org - Ticket created

Subject:

feature request: $ placeholders but not : placeholders

I'd like to be able to turn off : placeholders, but not ? placeholders. We're migrating a large code base from Pg to DBI / DBD::Pg, and I am running into a few queries that fail because they use : for array slices. I can get these queries to work by setting pg_placeholder_dollaronly, but my preference is to use ? for placeholders since seems to be more standard. I looked at the source code and I think I could probably do the legwork if nobody else has time to work on it, but I wanted to gauge possibly acceptance first.

Wed Apr 30 14:14:15 2014 greg [...] turnstep.com - Taken

Wed Apr 30 14:15:17 2014 greg [...] turnstep.com - Correspondence added

Can you show us a query that is failing? I want to make sure any tests we write are using queries that are failing in production.

Wed Apr 30 14:15:18 2014 The RT System itself - Status changed from 'new' to 'open'

Wed Apr 30 15:12:15 2014 PLICEASE [...] cpan.org - Correspondence added

On Wed Apr 30 14:15:17 2014, TURNSTEP wrote: Show quoted text

> Can you show us a query that is failing? I want to make sure any tests > we write are using queries that are failing in production.

Attaching a minimal script which demonstrates what I am seeing. Setting pg_placeholder_dollaronly to 1 makes it not die. We are using 2.19.3 in our environment, but I just double checked and I am seeing the same behavior in 3.1.1

Subject:

test.pl

use strict; use warnings; use v5.10; use Mojo::UserAgent; my $ua = Mojo::UserAgent->new; $ua->get("http://iscah.peanut.wdlabs.com", sub { my($ua, $tx) = @_; print $tx->req->to_string; print $tx->res->to_string; say 'done'; Mojo::IOLoop->stop; }); Mojo::IOLoop->start;

Wed Apr 30 15:13:08 2014 PLICEASE [...] cpan.org - Correspondence added

Sorry, that was the wrong test.pl :P This is the right one.

Subject:

test.pl

use strict; use warnings; use DBI; my $dbh = DBI->connect('dbi:Pg:dbname=ollisg', '','', { RaiseError => 1, pg_placeholder_dollaronly => 0 }); $dbh->do(q{ DROP TABLE IF EXISTS foo }); $dbh->do(q{ CREATE TABLE foo (bar text[]) }); my $sth = $dbh->prepare(q{ SELECT bar[1:2] FROM foo }); $sth->execute;

Thu May 01 11:23:25 2014 greg [...] turnstep.com - Correspondence added

On Wed Apr 30 15:13:08 2014, PLICEASE wrote: Show quoted text

> Sorry, that was the wrong test.pl :P > > This is the right one.

Thanks, I will play with this, but I think a better test would contain both : and ?, right? :)

Thu May 01 11:26:58 2014 greg [...] turnstep.com - Correspondence added

Show quoted text

> Thanks, I will play with this, but I think a better test would contain > both : and ?, right? :)

That is, for the failing test that we want to work post-patch: $dbh->do("CREATE TEMP TABLE foo( id INT, bar text[])"); $sth = $dbh->prepare(q{ SELECT bar[1:2] FROM foo WHERE is = ?});

Thu May 01 11:43:29 2014 PLICEASE [...] cpan.org - Correspondence added

On Thu May 01 11:26:58 2014, TURNSTEP wrote: Show quoted text

> > Thanks, I will play with this, but I think a better test would contain > > both : and ?, right? :)

> > That is, for the failing test that we want to work post-patch: > > $dbh->do("CREATE TEMP TABLE foo( id INT, bar text[])"); > $sth = $dbh->prepare(q{ SELECT bar[1:2] FROM foo WHERE is = ?}); >

That makes sense to me.

Thu May 01 12:08:33 2014 greg [...] turnstep.com - Correspondence added

I added some (currently failing) tests to t/12placeholders.t in 27f3088bdc061da72986b80e43b0640b9e42e22a I named the new var pg_placeholder_nocolons. Feel free to change it if you think of something better. Should be simple enough to implement, just copy all the code in dbdimp.c and dbdimp.h for placeholder_dollaronly. Doc updates in Pg.pm would be ideal too. Awaiting your patch :) Feel free to github pull request it if you like, or just post it here.

Fri May 02 07:06:06 2014 greg [...] turnstep.com - Correspondence added

On reflection, maybe we can just make the parser smarter to not consider #:# a placeholder. Seems array slices are the only time anyone has ever complained about placeholder/colon issues, so it may be okay to special case this. Would alienate those using #:# as placeholders, but that shouldn't work anyway, and even if it did, a whitespace would fix it "# :#".

Fri May 02 07:38:34 2014 PLICEASE [...] cpan.org - Correspondence added

On Fri May 02 07:06:06 2014, TURNSTEP wrote: Show quoted text

> On reflection, maybe we can just make the parser smarter to not > consider #:# a placeholder. Seems array slices are the only time > anyone has ever complained about placeholder/colon issues, so it may > be okay to special case this. Would alienate those using #:# as > placeholders, but that shouldn't work anyway, and even if it did, a > whitespace would fix it "# :#".

I think that it would have to also handle cases like this: ollisg=> select bar[(select 1):2] from foo; bar ----- (0 rows) There are probably other ways you can use an array slice without a literal # as the first operand. But if implemented correctly smarter parser would also be acceptable to me.

Fri May 02 07:47:11 2014 greg [...] turnstep.com - Correspondence added

Show quoted text

> ollisg=> select bar[(select 1):2] from foo;

Yeah, I was looking at gram.y and you can put any expression at all in there, plus whitespace. We're not going to reimplement the Postgres parser :) but perhaps it would be enough to say a placeholder is only recognized if there is whitespace before it? Most placeholders should have this anyway, as they tend to be in the where clause and usually are preceded by a keyword and a space. And you can always rewrite your array slices without a whitespace, as above.

Fri May 02 07:58:30 2014 PLICEASE [...] cpan.org - Correspondence added

On Fri May 02 07:47:11 2014, TURNSTEP wrote: Show quoted text

> Yeah, I was looking at gram.y and you can put any expression at all in > there, plus whitespace. We're not going to reimplement the Postgres > parser :) but perhaps it would be enough to say a placeholder is only > recognized if there is whitespace before it? Most placeholders should > have this anyway, as they tend to be in the where clause and usually > are preceded by a keyword and a space. And you can always rewrite your > array slices without a whitespace, as above.

Being restrictive with space in an array slice would not be my first choice. Space may be useful for making a query more readable. I could live with it though.

Fri May 02 10:29:09 2014 PLICEASE [...] cpan.org - Correspondence added

Another thing to consider maybe is I inserts: INSERT INTO foo (bar) VALUES (:1) would need to be rewritten as INSERT INTO foo (bar) VALUES ( :1) What would be common enough I think you'd want to add an exception for (:#

Fri May 02 10:52:18 2014 greg [...] turnstep.com - Correspondence added

Show quoted text

> What would be common enough I think you'd want to add an exception for (:#

What a big can of worms we've opened! Still, the :foo form is certainly not as popular as $1 (which Postgres uses internally) and ? (which is probably the most popular DBI form), so I'm willing to inflict some potential pain on people who are doing weird things with the :foo form. Worst case scenario, they can switch to another placeholder format and/or use pg_placeholder_dollaronly. I do agree your example is certainly something we should continue to support. Maybe I should run this by the list and see if others can think of examples or objections.

Tue May 06 16:33:56 2014 PLICEASE [...] cpan.org - Correspondence added

On Fri May 02 10:52:18 2014, TURNSTEP wrote: Show quoted text

> I do agree your example is certainly something we should continue to > support. Maybe I should run this by the list and see if others can > think of examples or objections.

Any update on this? Not exactly sure which list you had in mind, but I'd be interested in monitoring it if it is open, may even have $0.02 to throw in.

Tue May 06 17:24:56 2014 greg [...] turnstep.com - Correspondence added

Show quoted text

> Any update on this? Not exactly sure which list you had in mind, but > I'd be interested in monitoring it if it is open, may even have $0.02 > to throw in.

Haven't got around to it yet, but the list is dbd-pg@perl.org To subscribe, email dbd-pg-subscribe@perl.org

Tue May 06 17:33:44 2014 rlippan [...] remotelinux.com - Correspondence added

Subject:	Re: [rt.cpan.org #95173] feature request: $ placeholders but not : placeholders
Date:	Tue, 06 May 2014 22:24:12 +0100
To:	<bug-DBD-Pg [...] rt.cpan.org>
From:	Rudolf Lippan <rlippan [...] remotelinux.com>

On Tue, 6 May 2014 16:33:57 -0400, "Graham Ollis via RT" <bug-DBD-Pg@rt.cpan.org> wrote: Show quoted text

> Queue: DBD-Pg > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=95173 > > > On Fri May 02 10:52:18 2014, TURNSTEP wrote:

>> I do agree your example is certainly something we should continue to >> support. Maybe I should run this by the list and see if others can >> think of examples or objections.

> > Any update on this? Not exactly sure which list you had in mind, but

I'd I would assume DBD::Pg and DBI lists. Show quoted text

> be interested in monitoring it if it is open, may even have $0.02 to

throw Show quoted text

> in.

Perhaps I am a little late to the party here, but I am pretty sure that "$sth = $dbh->prepare(q{ SELECT bar[1:2] FROM foo WHERE is = ?});" was working at one point. Found it: 1.31 Mon Nov 17 21:21:21 UTC 2003 - Fixed statement scan problem where the preparse of "SELECT foo[3:33] from bar" was scanning :33 as a placeholder So you might be able to use an earlier version of DBD::Pg for right now. I'd offer to take a look, but I won't know for a few days whether I'd have to flexibility support any patch I come up with. I always wonder why the '?' style is so popular (other than DBD::mysql does not have support) because with :named placeholder it is easy to bind a whole hash to a statement.... -r

Thu May 08 07:29:40 2014 TIMB [...] cpan.org - Correspondence added

Parsing the statement is a slippery slope I'd be very cautious of stepping onto. Requiring a space before a colon is a non-starter, I think. Only recognizing colons if they are followed immediately by letters ([a-zA-Z]) is about as far as I'd be comfortable going. (Note that I've phrased it that way on purpose. Syntax like [-2:-1] shouldn't be treated as a placeholder.)

Thu May 08 11:34:18 2014 greg [...] turnstep.com - Correspondence added

I'm thinking we should do both: have a flag to absolutely ignore all colons, but make the parser smarter to not bind placeholders when part of array slices (as best it can). On Thu May 08 07:29:40 2014, TIMB wrote: Show quoted text

> Parsing the statement is a slippery slope I'd be very cautious of > stepping onto.

Way too late for that :) Show quoted text

> Requiring a space before a colon is a non-starter, I think.

Yes, as clean as the solution is, it would break a lot of code like this: SELECT count(*) FROM TABLE WHERE mycol=:foobar Show quoted text

> Only recognizing colons if they are followed immediately by letters > ([a-zA-Z]) is about as far as I'd be comfortable going. (Note that > I've phrased it that way on purpose. Syntax like [-2:-1] shouldn't be > treated as a placeholder.)

Too much code out there using :1 unfortunately. I'm okay with making the parser simply refuse placeholders if a number comes right before the colon. It won't account for all cases, but nothing will, really, short of reimplenting the Postgres parser. At any rate, let's get the new attrib in place.

Fri May 16 08:46:05 2014 PLICEASE [...] cpan.org - Correspondence added

we are now using 3.2.0 in our environment, which resolves this issue for me. I am leaving this open as you may want to use it to track the parsing issue. If not feel free to close it. Thanks!

Sun May 18 11:33:31 2014 greg [...] turnstep.com - Correspondence added

On Fri May 16 08:46:05 2014, PLICEASE wrote: Show quoted text

> we are now using 3.2.0 in our environment, which resolves this issue > for me. I am leaving this open as you may want to use it to track the > parsing issue. If not feel free to close it.

Cool. I just pushed a change to disallow the simple case of number-colon-number. 4c492806acda288fc10f0260c922c0a8a563920b

Sun May 18 11:44:34 2014 greg [...] turnstep.com - Status changed from 'open' to 'patched'

Wed May 21 06:01:23 2014 greg [...] turnstep.com - Fixed in 3.2.0 added

Wed May 21 06:01:25 2014 greg [...] turnstep.com - Fixed in 3.2.1 added

Wed May 21 06:01:41 2014 greg [...] turnstep.com - Status changed from 'patched' to 'resolved'