Re: Bug in AbstracJdbc2Statement.replaceProcessing when using dollar quoting?

Hi,

See below for my thoughts on how this is limited and inefficient.

> -----Original Message-----
> From: pgsql-jdbc-owner(at)postgresql(dot)org [mailto:pgsql-jdbc-
> owner(at)postgresql(dot)org] On Behalf Of marc(dot)mg75(at)googlemail(dot)com
> Sent: Tuesday, October 09, 2012 7:15 AM
> To: pgsql-jdbc(at)postgresql(dot)org
> Subject: Re: [JDBC] Bug in AbstracJdbc2Statement.replaceProcessing when
> using dollar quoting?
>
> Am Donnerstag, 27. September 2012 15:02:28 UTC+2 schrieb Florent
> Guillaume:
>
> > Oracle is well known for its V$SESSION and other similar tables.
>
> Hi,
> even if Oracle uses V$SESSIONs, the $ here is not at the beginning, and it
also
> does not fit the sql standard.
>
> I made a small patch doing what was said above. While parsing the sql code
it
> checks if there is a $ while in the state IN_SQLCODE, if so it stops the
parsing
> and returns the original sql.
>
> I ran the testsuite and it passed with no error. I also checked my example
and
> the result returned is what would be expected.
>
>
> --- /org/postgresql/jdbc2/AbstractJdbc2Statement.java_orig Tue Oct 09
> 10:25:10 2012
> +++ /org/postgresql/jdbc2/AbstractJdbc2Statement.java Tue Oct 09
> 10:31:50 2012
> @@ -895,6 +895,7 @@
> int len = p_sql.length();
> StringBuffer newsql = new StringBuffer(len);
> int i=0;
> + try {
> while (i<len){
>
> i=parseSql(p_sql,i,newsql,false,connection.getStandardConformingStrings())
> ;
> // We need to loop here in case we encounter invalid @@
-907,6
> +908,11 @@
> i++;
> }
> }
> + } catch (final PGDollarQuoteParsingException e) {
> + // found dollar quoting in the sql string.
do
> not parse for
> + // escape clauses and return the original
sql.
> + return p_sql;
> + }
> return newsql.toString();
> }
> else
> @@ -929,7 +935,7 @@
> * @return the position we stopped processing at
> */
> protected static int parseSql(String p_sql,int i,StringBuffer newsql,
boolean
> stopOnComma,
> - boolean stdStrings)throws SQLException{
> + boolean stdStrings)throws
> + SQLException, PGDollarQuoteParsingException {
> short state = IN_SQLCODE;
> int len = p_sql.length();
> int nestedParenthesis=0;
> @@ -955,6 +961,10 @@
> endOfNested=true;
> break;
> }
> + } else if (c == '$') { // start of a dollar quoted
string
> + // dollar quoted strings are postgreSql
only,
> throw an
> + // exception to stop parsing for db
> indepentend syntax.
> + throw new
> PGDollarQuoteParsingException();
> } else if (stopOnComma && c==',' && nestedParenthesis==0)
{
> endOfNested=true;
> break;
> @@ -1066,7 +1076,7 @@
> * @param stdStrings whether standard_conforming_strings is on
> * @return the right postgreSql sql
> */
> - protected static String escapeFunction(String functionName, String
args,
> boolean stdStrings) throws SQLException{
> + protected static String escapeFunction(String functionName, String
> + args, boolean stdStrings) throws SQLException,
> + PGDollarQuoteParsingException{
> // parse function arguments
> int len = args.length();
> int i=0;
>
>
>
>
> And i added an exception for this:
>
>
> package org.postgresql.core;
>
> public class PGDollarQuoteParsingException extends Exception {
>
> }
>
>
> Using this exception i handle to stop the parsing. It could also be used
to print
> a warning or whatever.
>

This patch appears to throw the exception when an unquoted identifier
includes a dollar-sign. I cannot speak to the standard but it would seem
that presence of the dollar-sign should only be evaluated if we are
"IN_SQLCODE" and the preceding character is NOT (alphanumeric or underscore)
{specifically whatever characters are allowed to begin an unquoted
identifier}.

Also, with the logic provided the addition of an exception and a try/catch
block appears to add unnecessary overhead. At the point the exception is
thrown I would suggest that we instead:

"return p_sql;"

and let the replaceProcessing method remain ignorant of whether the SQL it
is getting back is the original SQL or a modified version.

The fact is the presence of dollar-quoting with enabled escaping is going to
be the common situation. Combine that with the fact we are not prompting
the user to "fix" anything means that using the exception mechanism a poor
choice - it is not Exceptional but rather ordinary. Everything that needs
to be checked and confirmed can be done in the parseSQL method and since it
has access to the original statement it can return the original when
necessary.

David J.