Re: [pgsql-jdbc] dollar-quoted CREATE FUNCTION statement fails

Hi,

I've encountered a strange behavior of the JDBC driver 8.1.407 with PostgreSQL 8.1.4 (windows platform). I really suspect this is a bug inside the driver implementation, that's reason why I write this post.

Here is the use case :

I want to create a trigger with the JDBC API. This trigger function uses dollar-quoted escaped string literal. The reason for this is that I use a search_path variable for the connected user, and I want postgres to automatically add the correct db schema inside my trigger declaration. Here it is :

CREATE OR REPLACE FUNCTION procedure_insert_deleted_document() RETURNS TRIGGER AS $trigger_insert_deleted_document$
BEGIN INSERT INTO DELETED_DOCUMENT (DOC_ID, modificationDate, DOCVAULT_ID, guid) VALUES(OLD.DOC_ID, localtimestamp, OLD.DOCVAULT_ID, OLD.guid); RETURN OLD; END; $trigger_insert_deleted_document$ LANGUAGE plpgsql;
CREATE TRIGGER trigger_insert_deleted_document AFTER DELETE ON gch.DOCUMENT FOR EACH ROW EXECUTE PROCEDURE procedure_insert_deleted_document();

The problem is that this statement is internally broke-up into too many SimpleQuery objects by the driver. In fact, the $ sign escape doesn't seem to be recognize and the above statement is splitted up in five (after each semicolon):

CREATE OR REPLACE FUNCTION procedure_insert_deleted_document() RETURNS TRIGGER AS $trigger_insert_deleted_document$
BEGIN INSERT INTO DELETED_DOCUMENT (DOC_ID, modificationDate, DOCVAULT_ID, guid) VALUES(OLD.DOC_ID, localtimestamp, OLD.DOCVAULT_ID, OLD.guid)

RETURN OLD

END

$trigger_insert_deleted_document$ LANGUAGE plpgsql

CREATE TRIGGER trigger_insert_deleted_document AFTER DELETE ON gch.DOCUMENT FOR EACH ROW EXECUTE PROCEDURE procedure_insert_deleted_document()

Of course, the execution of these statement fails after the first one with the following error :

ERROR: unterminated dollar-quoted string at or near "$trigger_insert_deleted_document$ BEGIN INSERT INTO gch.DELETED_DOCUMENT (DOC_ID, modificationDate, DOCVAULT_ID, guid) VALUES(OLD.DOC_ID, localtimestamp, OLD.DOCVAULT_ID, OLD.guid)"

On the contrary, if I fall back to standard quoted string, the statement below is this time split in two SimpleQuery and succeeds.

CREATE OR REPLACE FUNCTION procedure_insert_deleted_document() RETURNS TRIGGER AS ' BEGIN INSERT INTO gch.DELETED_DOCUMENT (DOC_ID, modificationDate, DOCVAULT_ID, guid) VALUES(OLD.DOC_ID, localtimestamp, OLD.DOCVAULT_ID, OLD.guid); RETURN OLD; END; ' LANGUAGE plpgsql;
CREATE TRIGGER trigger_insert_deleted_document AFTER DELETE ON gch.DOCUMENT FOR EACH ROW EXECUTE PROCEDURE procedure_insert_deleted_document();

But this workaround doesn't suit me as it requires explicit schema prefix inside the trigger.

So my question is : Does this is a known issue of the JDBC driver or does it remind you something equivalent ?
(I can provide further details if needed)

Thanks for your answer

Regards,

Gregory

=?iso-8859-1?Q?Gr=E9gory_Chazalon?= <Gregory(dot)Chazalon(at)advestigo(dot)com> writes:
> I've encountered a strange behavior of the JDBC driver 8.1.407 with PostgreSQL 8.1.4 (windows platform). I really suspect this is a bug inside the driver implementation, that's reason why I write this post.

Last I heard, the JDBC driver hadn't been taught about dollar quoting at
all, so it's not surprising that a DQ literal within a query would
confuse it.

This ought to get fixed sometime ...

regards, tom lane

Thanks for your answer Tom.

It's too bad that the driver doesn't support this...
I have no idea how difficult would it be to include such an improvement.
However, do you think there is a chance to see this in one of the next releases ?

Needless to say it would be greatly appreciated, at least by one user :-)

Regards,

Gregory

-----Message d'origine-----
De : Tom Lane [mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us]
Envoyé : mardi 22 août 2006 15:10
À : Grégory Chazalon
Cc : pgsql-jdbc(at)postgresql(dot)org
Objet : Re: [JDBC] [pgsql-jdbc] dollar-quoted CREATE FUNCTION statement fails to execute (SimpleQuery splitting invalid)

=?iso-8859-1?Q?Gr=E9gory_Chazalon?= <Gregory(dot)Chazalon(at)advestigo(dot)com> writes:
> I've encountered a strange behavior of the JDBC driver 8.1.407 with PostgreSQL 8.1.4 (windows platform). I really suspect this is a bug inside the driver implementation, that's reason why I write this post.

Last I heard, the JDBC driver hadn't been taught about dollar quoting at all, so it's not surprising that a DQ literal within a query would confuse it.

This ought to get fixed sometime ...

regards, tom lane

About dollar-quoting in the JDBC driver...

Tom Lane wrote:
> Last I heard, the JDBC driver hadn't been taught about dollar quoting at
> all, so it's not surprising that a DQ literal within a query would
> confuse it.
>
> This ought to get fixed sometime ...

Grégory Chazalon write:
> It's too bad that the driver doesn't support this... I have no idea how
> difficult would it be to include such an improvement. However, do you
> think there is a chance to see this in one of the next releases ?
>
> Needless to say it would be greatly appreciated, at least by one user
> :-)

Me too.

I guess it's just a matter of coding, right? Would the JDBC maintainers
accept a patch for the 8.2 release of the driver?

Best Regards
Michael Paesold

On 28-Sep-06, at 7:13 AM, Michael Paesold wrote:

> About dollar-quoting in the JDBC driver...
>
> Tom Lane wrote:
> > Last I heard, the JDBC driver hadn't been taught about dollar
> quoting at
> > all, so it's not surprising that a DQ literal within a query would
> > confuse it.
> >
> > This ought to get fixed sometime ...
>
> Grégory Chazalon write:
>> It's too bad that the driver doesn't support this... I have no
>> idea how
>> difficult would it be to include such an improvement. However, do you
>> think there is a chance to see this in one of the next releases ?
>> Needless to say it would be greatly appreciated, at least by one user
>> :-)
>
> Me too.
>
> I guess it's just a matter of coding, right? Would the JDBC
> maintainers accept a patch for the 8.2 release of the driver?
Absolutely!
>
> Best Regards
> Michael Paesold
>
> ---------------------------(end of
> broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match
>

Dave Cramer wrote:
>> I guess it's just a matter of coding, right? Would the JDBC
>> maintainers accept a patch for the 8.2 release of the driver?
> Absolutely!

Ok, I am going to write support for this, but I need some help with
determining if a character is valid for a dollar-quote tag. I am
currently looking at the original patch implementing dollar-quoting in
the backend parser, see here:
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/scan.l.diff?r1=1.114;r2=1.115

scan.l defines:
dolq_start [A-Za-z\200-\377_]
dolq_cont [A-Za-z\200-\377_0-9]

Some questions here:
- What are the \200-\377 characters?

- Any ideas how to efficiently check this in Java?
There is Character.isLetter() etc., but this includes all Unicode
letters, not only letters valid as identifiers in the backend.

Otherwise I can still use something like
public static boolean isDollarQuoteStartChar(char c) {
return (c >= 'A' && c <= 'Z') || ...
}

I am currently working on dollar-quote support in
v3.QueryExecutorImpl.parseQuery(..), but I also want to add it to
V2Query. Where should I put such a static method like
isDollarQuoteStartChar? org.postgresql.core.Utils?

Best Regards
Michael Paesold

Michael Paesold <mpaesold(at)gmx(dot)at> writes:
> scan.l defines:
> dolq_start [A-Za-z\200-\377_]
> dolq_cont [A-Za-z\200-\377_0-9]

> Some questions here:
> - What are the \200-\377 characters?

Basically, that's going to cover any non-7-bit-ASCII character
(including multibyte characters). I'm not sure if Java has an
equivalent of ctype.h's isascii() but that'd probably be what
you want to use. Checking if the Unicode code point is > 127
would work too, if Java lets you do that.

regards, tom lane

Tom Lane schrieb:
> Michael Paesold <mpaesold(at)gmx(dot)at> writes:
>> scan.l defines:
>> dolq_start [A-Za-z\200-\377_]
>> dolq_cont [A-Za-z\200-\377_0-9]
>
>> Some questions here:
>> - What are the \200-\377 characters?
>
> Basically, that's going to cover any non-7-bit-ASCII character
> (including multibyte characters). I'm not sure if Java has an
> equivalent of ctype.h's isascii() but that'd probably be what
> you want to use. Checking if the Unicode code point is > 127
> would work too, if Java lets you do that.

Ok, CharUtils of commons-lang of Apache.org defines isAscii as simply as:

public static boolean isAscii(char ch) {
return (ch < 128);
}

So something like this should do the trick:

public static boolean isDollarQuoteStartChar(char c) {
return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')
|| (c == '_') || (c > 127);
}

(c > 127) should be the same as (c >= '\200'), but I find the first one
more readable. I am probably not used to reading hex numbers. ;-)

Thanks for your help.

Best Regards
Michael Paesold

Hi, Michael,

Michael Paesold wrote:

> (c > 127) should be the same as (c >= '\200'), but I find the first one
> more readable. I am probably not used to reading hex numbers. ;-)

Just to be picky: The "200" is octal, not hex. The hex value for 128 is
0x80. :-)

HTH,
Markus

Markus Schaber wrote:
> Michael Paesold wrote:
>
>> (c > 127) should be the same as (c >= '\200'), but I find the first one
>> more readable. I am probably not used to reading hex numbers. ;-)
>
> Just to be picky: The "200" is octal, not hex. The hex value for 128 is
> 0x80. :-)

You are right, of course. It caught my eye after sending the mail, but I
hoped nobody would notice. People are just to bright here. :-)

Best Regards
Michael Paesold

Michael Paesold wrote:
> Dave Cramer wrote:
> >> I guess it's just a matter of coding, right? Would the JDBC
> >> maintainers accept a patch for the 8.2 release of the driver?
> > Absolutely!
>
> Ok, I am going to write support for this... [snip]

Here is a first version of the patch. It works well for me so far, so I
am asking for comments now.

This patch only adds knowledge about dollar-quotes to
v3.QueryExecutorImpl.parseQuery() for now, but there are other possible
candidates:
- V2Query: for people connecting to postgres >= 8.0 using
?protocolVersion=2
- AbstractJdbc2Statement.modifyJdbcCall() for prepareCall: I would not
expect someone to use dollar-quoting here, but single-quotes are
supported, so adding dollar-quoting does not seem completely off
track.

I have tried to make the patch rather non-invasive. I thought about
improving parseQuery to be more efficient, but without having done any
profiling, I did not want to considerably change the existing code
paths. All JUnit tests still pass running against an 8.1 server.

p1-remove-unused-var.patch:

org/postgresql/core/v3/QueryExecutorImpl.java:
- Remove an unused variable from parseQuery

p2-v3protocol-dolquot.patch:

org/postgresql/core/Utils.java:
- Add static methods isDollarQuoteStartChar(char) and
isDollarQuoteContChar(char)

org/postgresql/core/v3/QueryExecutorImpl.java:
- Add support for dollar-quoting in parseQuery

org/postgresql/test/jdbc3/Jdbc3DollarQuotingTest.java:
- New file with JUnit tests for new code

org/postgresql/test/jdbc3/Jdbc3TestSuite.java:
- Add the new JUnit TestCase to the test suite if server >= 8.0 and
protocolVersion == 3

Best Regards
Michael Paesold

Michael Paesold wrote:
> Here is a first version of the patch. It works well for me so far, so
> I am asking for comments now.

Before everyone forgets about my patch in the discussion about the later
patch by Jure Koren...

I have already spent some hours on doing research, coding, creating the
test case. So I am willing to improve my work, of course. It just much
easier with input from the community. I would really welcome any
suggestions or criticism.

While at it, don't you think that we should handle SQL comments in
parseQuery? For example when loading a schema from a file using JDBC,
there might be comments in it that will break the current code. Does the
JDBC spec say anything about that matter?

One other thing that I wanted to try is to not parse the whole query in
a single loop, but instead split up the parsing a bit more, in the style of:

case '"': scan for next '"';
case '\'': scan for matching '\'';
case '$': if dollar-quote, scan for end of dollar-quote

It would simplify the other branches of the switch statement and we
could get rid of the if (!in... && !in...) stuff, then.

Do you think that would be an improvement?

Best Regards
Michael Paesold

On Tue, 3 Oct 2006, Michael Paesold wrote:

> While at it, don't you think that we should handle SQL comments in
> parseQuery? For example when loading a schema from a file using JDBC, there
> might be comments in it that will break the current code. Does the JDBC spec
> say anything about that matter?

I haven't looked at either dollar quote patch, but will hopefully get to
that and other JDBC items later this week. Not parsing comments, both --
and /* */ is a complaint we here as often if not if not with more
frequency than dollar quoting. (Apparently there's a Hibernate option
that puts things in comments.) So, yes that would be a good thing to look
at.

Another parsing problem that we'll have with the 8.2 release is the
standard_conforming_strings configuration variable. I would appreciate
anyone taking a look at that and the issues that it will create, not just
for query parsing, but also for escaping in the V2 path and
DatabaseMetaData methods.

Kris Jurka

Kris Jurka wrote:
> Not parsing comments, both -- and /* */ is a complaint we here as
> often if not if not with more frequency than dollar quoting.

Ok, I will update my dollar-quoting patch to handle comments, too. I am
going to update V2Query as well, then. Anything else that is missing
from the query parsing right now?

> Another parsing problem that we'll have with the 8.2 release is the
> standard_conforming_strings configuration variable. I would appreciate
> anyone taking a look at that and the issues that it will create, not
> just for query parsing, but also for escaping in the V2 path and
> DatabaseMetaData methods.

I was just going to open a new thread on this. :-) As I was reading
through the code regarding dollar-quoting, I became aware of the issue.
So yes, I am willing to help.

Best Regards
Michael Paesold

Kris Jurka wrote:
> I haven't looked at either dollar quote patch, but will hopefully get to
> that and other JDBC items later this week. Not parsing comments, both
> -- and /* */ is a complaint we here as often if not if not with more
> frequency than dollar quoting. (Apparently there's a Hibernate option
> that puts things in comments.) So, yes that would be a good thing to
> look at.

I thought I'd keep you posted on my progress. Here is an updated
work-in-progress patch implementing dollar-quotes, -- and /* */ quotes
(also with SQL compliant nesting, i.e. /* /* */ */). It replaces my last
one.

It passes all tests, including the newly created one. In addition to
supporting comments now, I have changed how we iterate over the
character array. Instead of having just one big loop parsing all
constructs, I have created tight loops for finding the end of certain
constructs, exactly for single, double, and dollar quotes, and comments.

This reduces the runtime with all new added "features" by ~10% in my
test case (see below), whereas my first implementation (as well as the
one from Jure Koren) had a worsening effect on runtime. Although the
code is longer because of the inner loops, I think it is not less
understandable, especially with all the new cases, which would add quite
much of if (!... && ! ...) otherwise.

I did the runtime tests using the attached ParserSpeedTest.java. It
reads a bunch of SQL files into memory and then runs the parser on them.
Example:
java ParserSpeedTest 10000 ../../app/schema/*.sql
(call parser 10000 times on all given *.sql files)

My todo list for this patch:
- Add the same capabilities to V2Query. Should I just copy the
corresponding parts of the code there? I don't see an easy way for code
sharing here. Although it would be possible to factor parts into extra
methods, I don't see that this would be a real win. Should I try? Comments?

- Move the tests from the new class to the places where they belong,
e.g. a testDollarQuotes method in the PreparedStatementTest class.

Comments?

Best Regards
Michael

On Fri, 6 Oct 2006, Michael Paesold wrote:

> I thought I'd keep you posted on my progress. Here is an updated
> work-in-progress patch implementing dollar-quotes, -- and /* */ quotes (also
> with SQL compliant nesting, i.e. /* /* */ */). It replaces my last one.

This looks good to me. Have you done anything further?

> - Add the same capabilities to V2Query. Should I just copy the corresponding
> parts of the code there? I don't see an easy way for code sharing here.
> Although it would be possible to factor parts into extra methods, I don't see
> that this would be a real win. Should I try? Comments?
>

Sharing would be good, but I agree there's no easy way for that to
work. Copying should be OK.

Kris Jurka

Kris Jurka wrote:
> On Fri, 6 Oct 2006, Michael Paesold wrote:
>
>> I thought I'd keep you posted on my progress. Here is an updated
>> work-in-progress patch implementing dollar-quotes, -- and /* */ quotes
>> (also with SQL compliant nesting, i.e. /* /* */ */). It replaces my
>> last one.
>
> This looks good to me. Have you done anything further?

Yeah, I have started extracting the code for parsing specific parts of the
query (single-quotes, dollar-quotes, comments, etc.) into a separate class
so the code can be reused in the V2Query code. I was also looking at
supporting standard_conforming_strings as a patch on top of that work.

> Sharing would be good, but I agree there's no easy way for that to
> work. Copying should be OK.

We will see shortly, if it works. I am going to post a new patch in the
next two days. Soon enough?

Best Regards
Michael Paesold

Michael Paesold wrote:
> Kris Jurka wrote:
>> On Fri, 6 Oct 2006, Michael Paesold wrote:
>>
>>> I thought I'd keep you posted on my progress. Here is an updated
>>> work-in-progress patch implementing dollar-quotes, -- and /* */
>>> quotes (also with SQL compliant nesting, i.e. /* /* */ */). It
>>> replaces my last one.
>>
>> This looks good to me. Have you done anything further?
>
> Yeah, I have started extracting the code for parsing specific parts of
> the query (single-quotes, dollar-quotes, comments, etc.) into a separate
> class so the code can be reused in the V2Query code. I was also looking
> at supporting standard_conforming_strings as a patch on top of that work.

Attached is a new version of the patch. From my side it looks pretty
complete now, but I am of course willing to improve it based on further
comments.

The parsing code is now split into several methods in a new class
org.postgresql.core.Parser. These methods are used in the v3
QueryExecutorImpl as well as the V2Query class. Therefore, both support
dollar-quotes and comments, now.

If you are OK with the approach, I will start coding the support for
standard_conforming_strings.

Best Regards,
Michael Paesold

Michael,

I'm only looking at this from a performance standpoint.

1) testing for array.length in a loop is fairly expensive compared to
testing for 0, not a big deal but it adds up.
2) I'm wondering what the cost of a switch is for two cases
( candidly I don't know the answer to this,and it's quite likely that
modern compilers will turn it into an if else anyway.)

Dave

On 1-Nov-06, at 4:47 PM, Michael Paesold wrote:

> Michael Paesold wrote:
>> Kris Jurka wrote:
>>> On Fri, 6 Oct 2006, Michael Paesold wrote:
>>>
>>>> I thought I'd keep you posted on my progress. Here is an updated
>>>> work-in-progress patch implementing dollar-quotes, -- and /* */
>>>> quotes (also with SQL compliant nesting, i.e. /* /* */ */). It
>>>> replaces my last one.
>>>
>>> This looks good to me. Have you done anything further?
>> Yeah, I have started extracting the code for parsing specific
>> parts of the query (single-quotes, dollar-quotes, comments, etc.)
>> into a separate class so the code can be reused in the V2Query
>> code. I was also looking at supporting standard_conforming_strings
>> as a patch on top of that work.
>
> Attached is a new version of the patch. From my side it looks
> pretty complete now, but I am of course willing to improve it based
> on further comments.
>
> The parsing code is now split into several methods in a new class
> org.postgresql.core.Parser. These methods are used in the v3
> QueryExecutorImpl as well as the V2Query class. Therefore, both
> support dollar-quotes and comments, now.
>
> If you are OK with the approach, I will start coding the support
> for standard_conforming_strings.
>
> Best Regards,
> Michael Paesold
> diff -crN pgjdbc.862ca68b9ea5/org/postgresql/core/Parser.java
> pgjdbc.323e7d139a7b/org/postgresql/core/Parser.java
> *** pgjdbc.862ca68b9ea5/org/postgresql/core/Parser.java 1970-01-01
> 01:00:00.000000000 +0100
> --- pgjdbc.323e7d139a7b/org/postgresql/core/Parser.java 2006-11-01
> 22:41:41.000000000 +0100
> ***************
> *** 0 ****
> --- 1,199 ----
> + /
> *---------------------------------------------------------------------
> ----
> + *
> + * Copyright (c) 2006, PostgreSQL Global Development Group
> + *
> + * IDENTIFICATION
> + * $PostgreSQL$
> + *
> +
> *---------------------------------------------------------------------
> ----
> + */
> + package org.postgresql.core;
> +
> + /**
> + * Basic query parser infrastructure.
> + *
> + * @author Michael Paesold (mpaesold(at)gmx(dot)at)
> + */
> + public class Parser {
> +
> + /**
> + * Find the end of the single-quoted string starting at the
> given offset.
> + */
> + public static int parseSingleQuotes(final char[] query, int
> offset) {
> + while (++offset < query.length)
> + {
> + switch (query[offset])
> + {
> + case '\\':
> + ++offset;
> + break;
> + case '\'':
> + return offset;
> + }
> + }
> + return query.length;
> + }
> +
> + /**
> + * Find the end of the double-quoted string starting at the
> given offset.
> + */
> + public static int parseDoubleQuotes(final char[] query, int
> offset) {
> + while (++offset < query.length && query[offset] != '"') ;
> + return offset;
> + }
> +
> + /**
> + * Test if the dollar character (<tt>$</tt>) at the given
> offset starts
> + * a dollar-quoted string and return the offset of the ending
> dollar
> + * character.
> + */
> + public static int parseDollarQuotes(final char[] query, int
> offset) {
> + if (offset + 1 < query.length)
> + {
> + int endIdx = -1;
> + if (query[offset + 1] == '$')
> + endIdx = offset + 1;
> + else if (isDollarQuoteStartChar(query[offset + 1]))
> + {
> + for (int d = offset + 2; d < query.length; ++d)
> + {
> + if (query[d] == '$')
> + {
> + endIdx = d;
> + break;
> + }
> + else if (!isDollarQuoteContChar(query[d]))
> + break;
> + }
> + }
> + if (endIdx > 0)
> + {
> + // found; note: tag includes start and end $
> character
> + int tagIdx = offset, tagLen = endIdx - offset + 1;
> + offset = endIdx; // loop continues at endIdx + 1
> + for (++offset; offset < query.length; ++offset)
> + {
> + if (query[offset] == '$' &&
> + subArraysEqual(query, tagIdx, offset,
> tagLen))
> + {
> + offset += tagLen - 1;
> + break;
> + }
> + }
> + }
> + }
> + return offset;
> + }
> +
> + /**
> + * Test if the <tt>-</tt> character at <tt>offset</tt> starts a
> + * <tt>--</tt> style line comment, and return the position of
> the first
> + * <tt>\r</tt> or <tt>\n</tt> character.
> + */
> + public static int parseLineComment(final char[] query, int
> offset) {
> + if (offset + 1 < query.length && query[offset + 1] == '-')
> + {
> + while (++offset < query.length)
> + {
> + if (query[offset] == '\r' || query[offset] == '\n')
> + break;
> + }
> + }
> + return offset;
> + }
> +
> + /**
> + * Test if the <tt>/</tt> character at <tt>offset</tt> starts
> a block
> + * comment, and return the position of the last <tt>/</tt>
> character.
> + */
> + public static int parseBlockComment(final char[] query, int
> offset) {
> + if (offset + 1 < query.length && query[offset + 1] == '*')
> + {
> + // /* /* */ */ nest, according to SQL spec
> + int level = 1;
> + for (offset += 2; offset < query.length; ++offset)
> + {
> + switch (query[offset-1]) {
> + case '*':
> + if (query[offset] == '/')
> + {
> + --level;
> + ++offset; // don't parse / in */* twice
> + }
> + break;
> + case '/':
> + if (query[offset] == '*')
> + {
> + ++level;
> + ++offset; // don't parse * in /*/ twice
> + }
> + break;
> + }
> + if (level == 0)
> + {
> + --offset; // reset position to last '/' char
> + break;
> + }
> + }
> + }
> + return offset;
> + }
> +
> + /**
> + * Checks if a character is valid as the start of a dollar
> quoting tag.
> + *
> + * @param c the character to check
> + * @return true if valid as first character of a dollar
> quoting tag; false if not
> + */
> + public static boolean isDollarQuoteStartChar(char c) {
> + /*
> + * The allowed dollar quote start and continuation
> characters
> + * must stay in sync with what the backend defines in
> + * pgsql/src/backend/parser/scan.l
> + */
> + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')
> + || c == '_' || c > 127;
> + }
> +
> + /**
> + * Checks if a character is valid as the second or latter
> character of a
> + * dollar quoting tag.
> + *
> + * @param c the character to check
> + * @return true if valid as second or later character of a
> dollar quoting tag;
> + * false if not
> + */
> + public static boolean isDollarQuoteContChar(char c) {
> + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')
> + || c == '_' || c > 127
> + || (c >= '0' && c <= '9');
> + }
> +
> + /**
> + * Compares two sub-arrays of the given character array for
> equalness.
> + * If the length is zero, the result is true, if and only if
> the offsets
> + * are within the bounds of the array.
> + *
> + * @param arr a char array
> + * @param offA first sub-array start offset
> + * @param offB second sub-array start offset
> + * @param len length of the sub arrays to compare
> + * @return true if the sub-arrays are equal; false if not
> + */
> + private static boolean subArraysEqual(final char[] arr,
> + final int offA, final
> int offB,
> + final int len) {
> + if (offA < 0 || offB < 0
> + || offA >= arr.length || offB >= arr.length
> + || offA + len > arr.length || offB + len >
> arr.length)
> + return false;
> +
> + for (int i = 0; i < len; ++i)
> + {
> + if (arr[offA + i] != arr[offB + i])
> + return false;
> + }
> +
> + return true;
> + }
> + }
> diff -crN pgjdbc.862ca68b9ea5/org/postgresql/core/v2/V2Query.java
> pgjdbc.323e7d139a7b/org/postgresql/core/v2/V2Query.java
> *** pgjdbc.862ca68b9ea5/org/postgresql/core/v2/V2Query.java
> 2006-11-01 22:41:41.000000000 +0100
> --- pgjdbc.323e7d139a7b/org/postgresql/core/v2/V2Query.java
> 2006-11-01 22:41:41.000000000 +0100
> ***************
> *** 29,62 ****
> Vector v = new Vector();
> int lastParmEnd = 0;
>
> ! boolean inSingleQuotes = false;
> ! boolean inDoubleQuotes = false;
>
> ! for (int i = 0; i < query.length(); ++i)
> {
> ! char c = query.charAt(i);
> !
> ! switch (c)
> {
> ! case '\\':
> ! if (inSingleQuotes)
> ! ++i; // Skip one character.
> break;
>
> ! case '\'':
> ! inSingleQuotes = !inDoubleQuotes && !inSingleQuotes;
> break;
>
> ! case '"':
> ! inDoubleQuotes = !inSingleQuotes && !inDoubleQuotes;
> break;
>
> case '?':
> ! if (!inSingleQuotes && !inDoubleQuotes)
> ! {
> ! v.addElement(query.substring (lastParmEnd, i));
> ! lastParmEnd = i + 1;
> ! }
> break;
>
> default:
> --- 29,63 ----
> Vector v = new Vector();
> int lastParmEnd = 0;
>
> ! char []aChars = query.toCharArray();
>
> ! for (int i = 0; i < aChars.length; ++i)
> {
> ! switch (aChars[i])
> {
> ! case '\'': // single-quotes
> ! i = Parser.parseSingleQuotes(aChars, i);
> ! break;
> !
> ! case '"': // double-quotes
> ! i = Parser.parseDoubleQuotes(aChars, i);
> break;
>
> ! case '-': // possibly -- style comment
> ! i = Parser.parseLineComment(aChars, i);
> break;
>
> ! case '/': // possibly /* */ style comment
> ! i = Parser.parseBlockComment(aChars, i);
> ! break;
> !
> ! case '$': // possibly dollar quote start
> ! i = Parser.parseDollarQuotes(aChars, i);
> break;
>
> case '?':
> ! v.addElement(query.substring (lastParmEnd, i));
> ! lastParmEnd = i + 1;
> break;
>
> default:
> diff -crN pgjdbc.862ca68b9ea5/org/postgresql/core/v3/
> QueryExecutorImpl.java pgjdbc.323e7d139a7b/org/postgresql/core/v3/
> QueryExecutorImpl.java
> *** pgjdbc.862ca68b9ea5/org/postgresql/core/v3/
> QueryExecutorImpl.java 2006-11-01 22:41:41.000000000 +0100
> --- pgjdbc.323e7d139a7b/org/postgresql/core/v3/
> QueryExecutorImpl.java 2006-11-01 22:41:41.000000000 +0100
> ***************
> *** 63,116 ****
> ArrayList statementList = new ArrayList();
> ArrayList fragmentList = new ArrayList(15);
>
> - boolean inQuotes = false;
> int fragmentStart = 0;
> -
> - boolean inSingleQuotes = false;
> - boolean inDoubleQuotes = false;
> int inParen = 0;
> !
> char []aChars = query.toCharArray();
> !
> for (int i = 0; i < aChars.length; ++i)
> {
> ! char c = aChars[i];
> !
> ! switch (c)
> {
> ! case '\\':
> ! if (inSingleQuotes)
> ! ++i; // Skip one character.
> break;
>
> ! case '\'':
> ! inSingleQuotes = !inDoubleQuotes && !inSingleQuotes;
> break;
>
> ! case '"':
> ! inDoubleQuotes = !inSingleQuotes && !inDoubleQuotes;
> break;
>
> ! case '?':
> ! if (withParameters && !inSingleQuotes && !
> inDoubleQuotes)
> ! {
> ! fragmentList.add(query.substring
> (fragmentStart, i));
> ! fragmentStart = i + 1;
> ! }
> break;
>
> case '(':
> ! if (!inSingleQuotes && !inDoubleQuotes)
> ! inParen++;
> break;
>
> case ')':
> ! if (!inSingleQuotes && !inDoubleQuotes)
> ! inParen--;
> break;
>
> case ';':
> ! if (!inSingleQuotes && !inDoubleQuotes && inParen
> == 0)
> {
> fragmentList.add(query.substring
> (fragmentStart, i));
> fragmentStart = i + 1;
> --- 63,115 ----
> ArrayList statementList = new ArrayList();
> ArrayList fragmentList = new ArrayList(15);
>
> int fragmentStart = 0;
> int inParen = 0;
> !
> char []aChars = query.toCharArray();
> !
> for (int i = 0; i < aChars.length; ++i)
> {
> ! switch (aChars[i])
> {
> ! case '\'': // single-quotes
> ! i = Parser.parseSingleQuotes(aChars, i);
> break;
>
> ! case '"': // double-quotes
> ! i = Parser.parseDoubleQuotes(aChars, i);
> break;
>
> ! case '-': // possibly -- style comment
> ! i = Parser.parseLineComment(aChars, i);
> break;
>
> ! case '/': // possibly /* */ style comment
> ! i = Parser.parseBlockComment(aChars, i);
> ! break;
> !
> ! case '$': // possibly dollar quote start
> ! i = Parser.parseDollarQuotes(aChars, i);
> break;
>
> case '(':
> ! inParen++;
> break;
>
> case ')':
> ! inParen--;
> ! break;
> !
> ! case '?':
> ! if (withParameters)
> ! {
> ! fragmentList.add(query.substring
> (fragmentStart, i));
> ! fragmentStart = i + 1;
> ! }
> break;
>
> case ';':
> ! if (inParen == 0)
> {
> fragmentList.add(query.substring
> (fragmentStart, i));
> fragmentStart = i + 1;
> diff -crN pgjdbc.862ca68b9ea5/org/postgresql/test/jdbc2/
> PreparedStatementTest.java pgjdbc.323e7d139a7b/org/postgresql/test/
> jdbc2/PreparedStatementTest.java
> *** pgjdbc.862ca68b9ea5/org/postgresql/test/jdbc2/
> PreparedStatementTest.java 2006-11-01 22:41:41.000000000 +0100
> --- pgjdbc.323e7d139a7b/org/postgresql/test/jdbc2/
> PreparedStatementTest.java 2006-11-01 22:41:41.000000000 +0100
> ***************
> *** 304,309 ****
> --- 304,374 ----
> pstmt.close();
> }
> }
> +
> + public void testDollarQuotes() throws SQLException {
> + // dollar-quotes are supported in the backend since
> version 8.0
> + if (!TestUtil.haveMinimumServerVersion(conn, "8.0"))
> + return;
> +
> + PreparedStatement st;
> + ResultSet rs;
> +
> + st = conn.prepareStatement("SELECT $$;$$ WHERE $x$?$x$=$_0
> $?$_0$ AND $$?$$=?");
> + st.setString(1, "?");
> + rs = st.executeQuery();
> + assertTrue(rs.next());
> + assertEquals(";", rs.getString(1));
> + assertFalse(rs.next());
> + st.close();
> +
> + st = conn.prepareStatement(
> + "SELECT $__$;$__$ WHERE ''''=$q_1$'$q_1$ AND
> ';'=?;"
> + + "SELECT $x$$a$;$x $a$$x$ WHERE $$;$$=? OR ''=$c
> $c$;$c$;"
> + + "SELECT ?");
> + st.setString(1, ";");
> + st.setString(2, ";");
> + st.setString(3, "$a$ $a$");
> +
> + assertTrue(st.execute());
> + rs = st.getResultSet();
> + assertTrue(rs.next());
> + assertEquals(";", rs.getString(1));
> + assertFalse(rs.next());
> +
> + assertTrue(st.getMoreResults());
> + rs = st.getResultSet();
> + assertTrue(rs.next());
> + assertEquals("$a$;$x $a$", rs.getString(1));
> + assertFalse(rs.next());
> +
> + assertTrue(st.getMoreResults());
> + rs = st.getResultSet();
> + assertTrue(rs.next());
> + assertEquals("$a$ $a$", rs.getString(1));
> + assertFalse(rs.next());
> + st.close();
> + }
> +
> + public void testComments() throws SQLException {
> + PreparedStatement st;
> + ResultSet rs;
> +
> + st = conn.prepareStatement("SELECT /*?*/ /*/*/*/**/*/*/*/
> 1;SELECT ?;--SELECT ?");
> + st.setString(1, "a");
> + assertTrue(st.execute());
> + assertTrue(st.getMoreResults());
> + assertFalse(st.getMoreResults());
> + st.close();
> +
> + st = conn.prepareStatement("SELECT /**/'?'/*/**/*/ WHERE
> '?'=/*/*/*?*/*/*/--?\n?");
> + st.setString(1, "?");
> + rs = st.executeQuery();
> + assertTrue(rs.next());
> + assertEquals("?", rs.getString(1));
> + assertFalse(rs.next());
> + st.close();
> + }
> +
> public void testDouble() throws SQLException
> {
> PreparedStatement pstmt = conn.prepareStatement("CREATE
> TEMP TABLE double_tab (max_double float, min_double float,
> null_value float)");
> diff -crN pgjdbc.862ca68b9ea5/org/postgresql/test/jdbc2/
> StatementTest.java pgjdbc.323e7d139a7b/org/postgresql/test/jdbc2/
> StatementTest.java
> *** pgjdbc.862ca68b9ea5/org/postgresql/test/jdbc2/
> StatementTest.java 2006-11-01 22:41:41.000000000 +0100
> --- pgjdbc.323e7d139a7b/org/postgresql/test/jdbc2/
> StatementTest.java 2006-11-01 22:41:41.000000000 +0100
> ***************
> *** 390,395 ****
> --- 390,442 ----
> assertTrue(!rs.next());
> }
>
> + public void testParsingDollarQuotes() throws SQLException
> + {
> + // dollar-quotes are supported in the backend since
> version 8.0
> + if (!TestUtil.haveMinimumServerVersion(con, "8.0"))
> + return;
> +
> + Statement st = con.createStatement();
> + ResultSet rs;
> +
> + rs = st.executeQuery("SELECT '$a$ ; $a$'");
> + assertTrue(rs.next());
> + assertEquals("$a$ ; $a$", rs.getObject(1));
> + rs.close();
> +
> + rs = st.executeQuery("SELECT $$;$$");
> + assertTrue(rs.next());
> + assertEquals(";", rs.getObject(1));
> + rs.close();
> +
> + rs = st.executeQuery("SELECT $OR$$a$'$b$a$$OR$ WHERE '$a
> $''$b$a$'=$OR$$a$'$b$a$$OR$OR ';'=''");
> + assertTrue(rs.next());
> + assertEquals("$a$'$b$a$", rs.getObject(1));
> + assertFalse(rs.next());
> + rs.close();
> +
> + rs = st.executeQuery("SELECT $B$;$b$B$");
> + assertTrue(rs.next());
> + assertEquals(";$b", rs.getObject(1));
> + rs.close();
> +
> + rs = st.executeQuery("SELECT $c$c$;$c$");
> + assertTrue(rs.next());
> + assertEquals("c$;", rs.getObject(1));
> + rs.close();
> +
> + rs = st.executeQuery("SELECT $A0$;$A0$ WHERE ''=$t$t$t$
> OR ';$t$'=';$t$'");
> + assertTrue(rs.next());
> + assertEquals(";", rs.getObject(1));
> + assertFalse(rs.next());
> + rs.close();
> +
> + st.executeQuery("SELECT /* */$$;$$/**//*;*/").close();
> + st.executeQuery("SELECT /* */--;\n$$a$$/**/--\n--;
> \n").close();
> +
> + st.close();
> + }
> +
> public void testUnbalancedParensParseError() throws SQLException
> {
> Statement stmt = con.createStatement();

Hi Dave,

first, I forgot to say that I did do performance tests using the
previously posted test case. The new version is definitely faster than
the original code, although only marginally. When doing my first version
of the patch, I tested different code constructs for speed. I chose the
one with the best performance.

I ran the tests with both 1.4.2 and 1.5.0. It might be possible that the
performance will be worse in an earlier versions of the JDK with a less
sophisticated hot-spot compiler.

Dave Cramer wrote:
> Michael,
>
> I'm only looking at this from a performance standpoint.
>
> 1) testing for array.length in a loop is fairly expensive compared to
> testing for 0, not a big deal but it adds up.

Could you give me a hint what part of the code you were looking at? And
what you would want me to change? Do you think we should scan the query
backwards (testing the offset against zero -- not sure if this is a good
idea) or do you want to make the query char[] zero-terminated so that we
can test against '\0'?

Btw. changing the code to
int len = array.length;
for (....; i < len ; ...)
did not give any performance advantage for me. I think the compiler can
quite effectively optimize access to .length here.

> 2) I'm wondering what the cost of a switch is for two cases ( candidly I
> don't know the answer to this,and it's quite likely that modern
> compilers will turn it into an if else anyway.)

Hmm, perhaps it's bad coding style that I left out the empty default.
But I only see this example in the code (2 cases + an implicit default):

+ switch (query[offset])
+ {
+ case '\\':
+ ++offset;
+ break;
+ case '\'':
+ return offset;
+ }

That would really translate to:
if (query[offset] = '\\') ... else if (query[offset] = '\'') ... /* else
nothing */

Should I add an empty default in that code sections? When I originally
wrote that code, it was all in that main loop, so I wanted to keep it
short. Now that it is split into several methods, I see no reason for
brevity.

Best Regards,
Michael Paesold

On 1-Nov-06, at 8:19 PM, Michael Paesold wrote:

> Hi Dave,
>
> first, I forgot to say that I did do performance tests using the
> previously posted test case. The new version is definitely faster
> than the original code, although only marginally. When doing my
> first version of the patch, I tested different code constructs for
> speed. I chose the one with the best performance.
>
> I ran the tests with both 1.4.2 and 1.5.0. It might be possible
> that the performance will be worse in an earlier versions of the
> JDK with a less sophisticated hot-spot compiler.

Cool my comments were pretty minor anyway.
>
> Dave Cramer wrote:
>> Michael,
>> I'm only looking at this from a performance standpoint.
>> 1) testing for array.length in a loop is fairly expensive compared
>> to testing for 0, not a big deal but it adds up.
>
> Could you give me a hint what part of the code you were looking at?
> And what you would want me to change? Do you think we should scan
> the query backwards (testing the offset against zero -- not sure if
> this is a good idea) or do you want to make the query char[] zero-
> terminated so that we can test against '\0'?
>
> Btw. changing the code to
> int len = array.length;
> for (....; i < len ; ...)
> did not give any performance advantage for me. I think the compiler
> can quite effectively optimize access to .length here.
>
>
>> 2) I'm wondering what the cost of a switch is for two cases
>> ( candidly I don't know the answer to this,and it's quite likely
>> that modern compilers will turn it into an if else anyway.)
>
> Hmm, perhaps it's bad coding style that I left out the empty
> default. But I only see this example in the code (2 cases + an
> implicit default):
>
> + switch (query[offset])
> + {
> + case '\\':
> + ++offset;
> + break;
> + case '\'':
> + return offset;
> + }
>
> That would really translate to:
> if (query[offset] = '\\') ... else if (query[offset] = '\'') ... /*
> else nothing */
>
> Should I add an empty default in that code sections? When I
> originally wrote that code, it was all in that main loop, so I
> wanted to keep it short. Now that it is split into several methods,
> I see no reason for brevity.
>
> Best Regards,
> Michael Paesold
>
>
>
>
>

Dave Cramer wrote:
> On 1-Nov-06, at 8:19 PM, Michael Paesold wrote:
>
>> Hi Dave,
>>
>> first, I forgot to say that I did do performance tests using the
>> previously posted test case. The new version is definitely faster than
>> the original code, although only marginally. When doing my first
>> version of the patch, I tested different code constructs for speed. I
>> chose the one with the best performance.
>>
>> I ran the tests with both 1.4.2 and 1.5.0. It might be possible that
>> the performance will be worse in an earlier versions of the JDK with a
>> less sophisticated hot-spot compiler.
>
> Cool my comments were pretty minor anyway.

Nevertheless, attached is an updated patch that adds the two missing
"default: break;" cases to the switch statements for better readability.

Best Regards
Michael Paesold

On Thu, 2 Nov 2006, Michael Paesold wrote:

> Nevertheless, attached is an updated patch that adds the two missing
> "default: break;" cases to the switch statements for better readability.
>

Applied. Thanks.

Kris Jurka