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

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
Index: core/Utils.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/core/Utils.java,v
retrieving revision 1.4
diff -c -r1.4 Utils.java
*** core/Utils.java    11 Jan 2005 08:25:43 -0000    1.4
--- core/Utils.java    6 Oct 2006 07:37:45 -0000
***************
*** 53,56 ****
--- 53,81 ----
              throw new RuntimeException("Unexpected exception: UTF-8 charset not supported: " + e);
          }
      }
+
+     /**
+      * 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) {
+         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');
+     }
  }
Index: core/v3/QueryExecutorImpl.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/core/v3/QueryExecutorImpl.java,v
retrieving revision 1.31
diff -c -r1.31 QueryExecutorImpl.java
*** core/v3/QueryExecutorImpl.java    7 Jul 2006 01:12:23 -0000    1.31
--- core/v3/QueryExecutorImpl.java    6 Oct 2006 07:37:46 -0000
***************
*** 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,195 ----
          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
!                 boolean done = false;
!                 while (!done && ++i < aChars.length)
!                 {
!                     switch (aChars[i])
!                     {
!                     case '\\':
!                         ++i;
!                         break;
!                     case '\'':
!                         done = true;
!                         break;
!                     }
!                 }
                  break;

!             case '"': // double-quotes
!                 while (++i < aChars.length && aChars[i] != '"') ;
                  break;

!             case '-': // -- style comments
!                 if (i + 1 < aChars.length && aChars[i + 1] == '-')
!                 {
!                     while (++i < aChars.length)
!                     {
!                         if (aChars[i] == '\r' || aChars[i] == '\n')
!                             break;
!                     }
!                 }
                  break;

!             case '/': // /* */ style comments
!                 if (i + 1 < aChars.length && aChars[i + 1] == '*')
                  {
!                     // /* /* */ */ nest, according to SQL spec
!                     int level = 1;
!                     for (i += 2; i < aChars.length; ++i)
!                     {
!                         switch (aChars[i-1]) {
!                         case '*':
!                             if (aChars[i] == '/')
!                             {
!                                 --level;
!                                 ++i; // don't parse / in */* twice
!                             }
!                             break;
!                         case '/':
!                             if (aChars[i] == '*')
!                             {
!                                 ++level;
!                                 ++i; // don't parse * in /*/ twice
!                             }
!                             break;
!                         }
!                         if (level == 0)
!                         {
!                             --i; // reset position to last '/' char
!                             break;
!                         }
!                     }
!                 }
!                 break;
!
!             case '$': // possible dollar quote start
!                 if (i + 1 < aChars.length)
!                 {
!                     int endIdx = -1;
!                     if (aChars[i + 1] == '$')
!                         endIdx = i + 1;
!                     else if (Utils.isDollarQuoteStartChar(aChars[i + 1]))
!                     {
!                         for (int d = i + 2; d < aChars.length; ++d)
!                         {
!                             if (aChars[d] == '$')
!                             {
!                                 endIdx = d;
!                                 break;
!                             }
!                             else if (!Utils.isDollarQuoteContChar(aChars[d]))
!                                 break;
!                         }
!                     }
!                     if (endIdx > 0)
!                     {
!                         // found; note: tag includes start and end $ character
!                         String tag = query.substring(i, endIdx + 1);
!                         i = endIdx; // loop continues at endIdx + 1
!                         for (++i; i < aChars.length; ++i)
!                         {
!                             if (aChars[i] == '$' && i + tag.length() <= aChars.length &&
!                                 query.substring(i, i + tag.length()).equals(tag))
!                             {
!                                 i += tag.length() - 1;
!                                 break;
!                             }
!                         }
!                     }
                  }
                  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;
Index: test/jdbc3/Jdbc3TestSuite.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/test/jdbc3/Jdbc3TestSuite.java,v
retrieving revision 1.16
diff -c -r1.16 Jdbc3TestSuite.java
*** test/jdbc3/Jdbc3TestSuite.java    15 May 2006 09:35:57 -0000    1.16
--- test/jdbc3/Jdbc3TestSuite.java    6 Oct 2006 07:37:46 -0000
***************
*** 34,39 ****
--- 34,43 ----
              {
                  suite.addTestSuite(Jdbc3CallableStatementTest.class);
              }
+             if ( TestUtil.haveMinimumServerVersion(con, "8.0") && TestUtil.isProtocolVersion(con, 3))
+             {
+                 suite.addTestSuite(Jdbc3ParserTest.class);
+             }
              con.close();
          }
          catch (Exception ex )
Index: test/jdbc3/Jdbc3ParserTest.java
===================================================================
RCS file: test/jdbc3/Jdbc3ParserTest.java
diff -N test/jdbc3/Jdbc3ParserTest.java
*** /dev/null    1 Jan 1970 00:00:00 -0000
--- test/jdbc3/Jdbc3ParserTest.java    1 Jan 1970 00:00:00 -0000
***************
*** 0 ****
--- 1,152 ----
+ /*-------------------------------------------------------------------------
+  *
+  * Copyright (c) 2006, PostgreSQL Global Development Group
+  *
+  * IDENTIFICATION
+  *   $PostgreSQL$
+  *
+  *-------------------------------------------------------------------------
+  */
+ package org.postgresql.test.jdbc3;
+
+ import org.postgresql.test.TestUtil;
+
+ import java.sql.*;
+
+ import junit.framework.TestCase;
+
+ /*
+  * Test for parser: quoting, dollar-quoting, comments, etc. in query
+  */
+ public class Jdbc3ParserTest extends TestCase {
+
+     private Connection con;
+
+     public Jdbc3ParserTest(String name) {
+         super(name);
+     }
+
+     protected void setUp() throws Exception
+     {
+         con = TestUtil.openDB();
+         TestUtil.createTable(con, "testdquot", "t text");
+         Statement stmt = con.createStatement();
+
+         stmt.executeUpdate(TestUtil.insertSQL("testdquot", "';'"));
+         stmt.executeUpdate(TestUtil.insertSQL("testdquot", "'?'"));
+         stmt.executeUpdate(TestUtil.insertSQL("testdquot", "'$a$ $a$'"));
+         stmt.executeUpdate(TestUtil.insertSQL("testdquot", "'$a$''$b$a$'"));
+         stmt.executeUpdate(TestUtil.insertSQL("testdquot", "';$b'"));
+         stmt.executeUpdate(TestUtil.insertSQL("testdquot", "'c$;'"));
+
+         stmt.close();
+     }
+
+     protected void tearDown() throws Exception
+     {
+         TestUtil.dropTable(con, "testdquot");
+         TestUtil.closeDB(con);
+     }
+
+     public void testSimple() throws SQLException
+     {
+         Statement st = con.createStatement();
+         ResultSet rs;
+
+         rs = st.executeQuery("SELECT t FROM testdquot WHERE t='$a$ $a$'");
+         assertTrue(rs.next());
+         assertEquals("$a$ $a$", rs.getObject(1));
+         rs.close();
+
+         rs = st.executeQuery("SELECT t FROM testdquot WHERE t=$$;$$");
+         assertTrue(rs.next());
+         assertEquals(";", rs.getObject(1));
+         rs.close();
+
+         rs = st.executeQuery("SELECT t FROM testdquot WHERE t=$OR$$a$'$b$a$$OR$OR t=';;'");
+         assertTrue(rs.next());
+         assertEquals("$a$'$b$a$", rs.getObject(1));
+         assertFalse(rs.next());
+         rs.close();
+
+         rs = st.executeQuery("SELECT t FROM testdquot WHERE t=$B$;$b$B$");
+         assertTrue(rs.next());
+         assertEquals(";$b", rs.getObject(1));
+         rs.close();
+
+         rs = st.executeQuery("SELECT t FROM testdquot WHERE t=$c$c$;$c$");
+         assertTrue(rs.next());
+         assertEquals("c$;", rs.getObject(1));
+         rs.close();
+
+         rs = st.executeQuery("SELECT t FROM testdquot WHERE t=$t$t$t$ OR t=';'");
+         assertTrue(rs.next());
+         assertEquals(";", rs.getObject(1));
+         assertFalse(rs.next());
+         rs.close();
+
+         st.executeUpdate("UPDATE testdquot SET t=';;' WHERE t=$q0$;$q0$;"
+                 + "UPDATE testdquot SET t='' WHERE t=';;'");
+         rs = st.executeQuery("SELECT t FROM testdquot WHERE t=''");
+         assertTrue(rs.next());
+         assertEquals("", rs.getObject(1));
+         assertFalse(rs.next());
+         rs.close();
+
+         // we only check if the backend does not report an error here,
+         // e.g., because we split the query too often
+         st.executeQuery("SELECT /* */$$;$$/**/").close();
+         st.executeQuery("SELECT /* */--;\n$$a$$/**/--\n--;\n").close();
+         //SELECT /* */--;\n$$a$$/**/--\n--;\n
+
+         st.close();
+     }
+
+     public void testParameterized() throws SQLException
+     {
+         PreparedStatement st;
+         ResultSet rs;
+
+         st = con.prepareStatement("SELECT /*/*/*/**/*/*/*/1;SELECT ?;");
+         st.setString(1, "a");
+         assertTrue(st.execute());
+         assertTrue(st.getMoreResults());
+         assertFalse(st.getMoreResults());
+         st.close();
+
+         st = con.prepareStatement("SELECT t FROM testdquot WHERE t=$$?$$ AND t=?");
+         st.setString(1, "?");
+         rs = st.executeQuery();
+         assertTrue(rs.next());
+         assertEquals("?", rs.getString(1));
+         assertFalse(rs.next());
+         st.close();
+
+         st = con.prepareStatement("SELECT t FROM testdquot WHERE t=$q_1$'$q_1$ OR t=?;"
+                 + "SELECT t FROM testdquot WHERE t=? OR t=$c$c$;$c$;"
+                 + "SELECT t FROM testdquot WHERE t=?");
+         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());
+         assertTrue(rs.next());
+         assertFalse(rs.next());
+
+         assertTrue(st.getMoreResults());
+         rs = st.getResultSet();
+         assertTrue(rs.next());
+         assertEquals("$a$ $a$", rs.getString(1));
+         assertFalse(rs.next());
+         st.close();
+     }
+
+ }
import org.postgresql.core.v3.QueryExecutorImpl;

import java.io.*;
import java.math.BigDecimal;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.Properties;

public class ParserSpeedTest {

    List queries;

    public ParserSpeedTest(List queries) {
        this.queries = queries;
    }

    public void run(int count)
    {
        QueryExecutorImpl q = new QueryExecutorImpl(null, null,
                new Properties(), null);
        for (int c = 0; c < count; ++c)
        {
            for (Iterator i = queries.iterator(); i.hasNext();)
            {
                String query = (String) i.next();
                q.createSimpleQuery(query);
            }
        }
    }

    public static void main(String[] args)
    {
        List queries = new LinkedList();

        if (args.length < 2)
        {
            System.out.println("Usage: java ParserSpeedTest count file.sql,...");
            System.exit(1);
        }

        int count = 1;

        try
        {
            count = Integer.parseInt(args[0]);

            for (int i = 1; i < args.length; i++)
            {
                String fname = args[i];
                File file = new File(fname);
                if (!file.exists() || !file.canRead())
                {
                    System.err.println("Argument \"" + fname
                            + "\" is not a valid file name");
                }

                StringWriter w = new StringWriter();
                Reader r = new FileReader(file);
                char[] buf = new char[2048];
                for (int read; (read = r.read(buf)) > 0;)
                {
                    w.write(buf, 0, read);
                }
                queries.add(w.getBuffer().toString());
            }
        } catch (IOException e)
        {
            System.err.println(e.getClass().getName() + ": " + e.getMessage());
            System.exit(1);
        }
        catch (NumberFormatException e)
        {
            System.err.println("First argument must be a number: " + args[0]);
            System.exit(1);
        }

        ParserSpeedTest test = new ParserSpeedTest(queries);
        long start = System.currentTimeMillis();
        test.run(count);
        long end = System.currentTimeMillis();
        long total = end - start;
        double time = ((double)total) / 1000;

        System.out.println("Files: " + queries.size() + "  Runs: " + count);
        System.out.println("Time: "
                + new BigDecimal(time).setScale(3, BigDecimal.ROUND_HALF_UP)
                + " s");

    }
}