#762 fix sql API for auto-generated keys

cheeser Sun 27 Sep 2009

I've run into a couple of issues the last few days and I've had to hack my fan installation. Below are the diffs of the changes I've made. Not all of them belong in the main distro as such but serve to show the hurdles i've encountered. I know the launcher stuff is currently looking at a bit of a redesign so my diffs to fanlauncher are probably not that big a deal but maybe can help someone else in the interim. The big change, for me, is in StatementPeer as the current (1.0.45 at least) code breaks under postgresql. Anyway, here are the diffs:

diff -ur fan-1.0.45/bin/fanlaunch rel/bin/fanlaunch
--- fan-1.0.45/bin/fanlaunch	2009-07-08 12:26:48.000000000 -0500
+++ rel/bin/fanlaunch	2009-09-16 13:35:37.000000000 -0500
@@ -75,6 +75,16 @@
     done  
   fi  

+  # add every jar file in lib/java/ext/{os} to cp
+  extDir="$FAN_REPO"/lib/java/
+  if [ -d $extDir ] ; then
+    for ext in `ls -b "$extDir"` ; do
+      if [ ${ext##*.} == "jar" ] ; then
+        FAN_CP="$FAN_CP:$extDir/$ext"
+      fi
+    done  
+  fi  
+  
 fi

 # Set FAN_JAVA if not already set
@@ -95,4 +105,4 @@
   FAN_MAIN=fanx.tools.$1
   shift
   $FAN_JAVA $osFlags $libPath -cp "$FAN_CP" -Dfan.home="$FAN_HOME" $FAN_MAIN "$@"
-}
\ No newline at end of file
+}
diff -ur fan-1.0.45/lib/sys.props rel/lib/sys.props
--- fan-1.0.45/lib/sys.props	2009-07-22 11:19:16.000000000 -0500
+++ rel/lib/sys.props	2009-09-20 14:54:31.000000000 -0500
@@ -34,7 +34,8 @@
 //////////////////////////////////////////////////////////////////////////

 // drivers
-sql.mysql.driver=com.mysql.jdbc.Driver
+//sql.mysql.driver=com.mysql.jdbc.Driver
+sql.postgresql.driver=org.postgresql.Driver

 // test setup
 sql.test.connection=jdbc:mysql://localhost:3306/fantest
@@ -60,4 +61,4 @@
   /C:/dev/fan/src/sys/dotnet/build.fan   = C:\\dev\\rel  \
   /C:/dev/fan/src/compiler/build.fan     = C:\\dev\\rel  \
   /C:/dev/fan/src/compilerJava/build.fan = C:\\dev\\rel  \
\-  /C:/dev/fan/src/build/build.fan        = C:\\dev\\rel
\ No newline at end of file
+  /C:/dev/fan/src/build/build.fan        = C:\\dev\\rel
diff -ur fan-1.0.45/src/sql/java/StatementPeer.java rel/src/sql/java/StatementPeer.java
--- fan-1.0.45/src/sql/java/StatementPeer.java	2009-07-08 12:27:02.000000000 -0500
+++ rel/src/sql/java/StatementPeer.java	2009-09-26 22:32:41.000000000 -0500
@@ -36,7 +36,13 @@
     }
     catch (SQLException ex)
     {
\-      throw ConnectionPeer.err(ex);
+        try {
+            stmt = self.conn.peer.jconn.prepareStatement(translated);
+            prepared = true;
+        }
+        catch (SQLException ex2) {
+            throw ConnectionPeer.err(ex2);
+        }
     }
     return self;
   }

brian Tue 29 Sep 2009

@cheeser,

I think first two diffs will be solved by existing tickets already pending for a fix.

The last issue is something new. Are you saying that postgresql throws an exception if you attempt to prepare statement using the Statement.RETURN_GENERATED_KEYS flag? Can you explain what you are seeing?

BTW - looking at the JavaDoc, the way java.sql.Connection overloads prepareStatement is insanely confusing

cheeser Tue 29 Sep 2009

Yeah, with postgresql that returns sql::SqlErr: Returning autogenerated keys is not supported. This is using postgresql-8.3-603.jdbc4.jar Now, the program seems to continue on as I'm seeing my objects load from the db but that's still a problem.

brian Tue 29 Sep 2009

OK, going thru this API and code I am going to propose the following change:

  1. Remove sql::SqlService.lastAutoKey, this is sort of a hackish place to get the auto-generated key
  2. create a new Statement.insert method which works just like sql::Statement.execute, but instead of returning the number of rows modified it returns the auto-generated key

If you don't use or care about auto-generated keys, then you can continue to use execute. If you do care about auto-generated keys then you would use the insert method which would throw an SqlErr if they were not supported by the underlying JDBC driver.

cheeser Tue 29 Sep 2009

In JDBC, there's http://java.sun.com/javase/6/docs/api/java/sql/Statement.html#getGeneratedKeys%28%29 that does that. It seems like putting that method on SqlService would horribly thread unsafe. I'm fine with adding an insert but I'm not sure that approach is required because of this issue. One potential problem with return an Int for the key is that it precludes batch inserts since only one Int would be returned. Does fan's API even support batching? I don't recall seeing that as an option in the API...

brian Tue 29 Sep 2009

Well it wasn't thread unsafe, b/c everything in that API uses thread locals to maintain one "Connection" per thread.

I actually never knew about getGeneratedKeys, so lets say Statement.insert returns an Obj[].

I think generated-keys is the fundamental issue here, which is why I proposed a new API. That way we only need to attempt use of the RETURN_GENERATED_KEYS flag in one single place (right now it is scattered around).

cheeser Tue 29 Sep 2009

That works for me. Unless I'm missing something, fan doesn't support batched ops at the moment anyway but maybe returning Int[] future proofs it against that? Or perhaps insert still just returns Int and later there'd be new methods to add/execute batch statements and a getGeneratedKeys() analog...

brian Wed 30 Sep 2009

Unless I'm missing something, fan doesn't support batched ops at the moment anyway but maybe returning Int[] future proofs it against that?

What are batched ops in JDBC? Aren't they just compound insert statements committed together? Or is there something missing from Fan's API?

brian Thu 1 Oct 2009

Promoted to ticket #762 and assigned to brian

brian Thu 1 Oct 2009

Renamed from launcher and sql updates to fix sql API for auto-generated keys

cheeser Thu 1 Oct 2009

if you look at http://java.sun.com/javase/6/docs/api/index.html?java/sql/Statement.html and http://java.sun.com/javase/6/docs/api/index.html?java/sql/PreparedStatement.html you'll see the batch methods there. With the prep'd statement, e.g., you can set params, addBatch(), clear(), and repeat then send them all off at once.

brian Thu 1 Oct 2009

OK, should we assume batch update is required?

Any suggestions on how that should look in Fan APIs?

cheeser Thu 1 Oct 2009

I'm sure it'll come up eventually for sure. In tests I've run in the past, I've seen huge gains when batching. As for how the fan API should look, i'm not sure. how about:

Void Statement.addBatch(Str:Obj params)
Int[] Statement.executeBatch

?

msl Thu 18 Feb 2010

Any progress on the RETURN_GENERATED_KEYS of this issue? I've just bumped into the same issue trying to port an app across from MySQL to PostgreSQL...

brian Thu 18 Feb 2010

Ticket resolved in 1.0.52

OK, I reworked things a bit:

  • I removed SqlService.lastAutoKey
  • I changed Statement.execute to return an Obj
  • If there are auto-generated keys, execute returns Int[] instead of of row count

changeset

Martin, can you try out that patch and let me know how it works for you?

msl Thu 18 Feb 2010

Thanks Brian - will take a look at it tonight.

msl Fri 19 Feb 2010

Still seeing the same issue:

sql::SqlErr: Returning autogenerated keys is not supported.
  sql::ConnectionPeer.err (ConnectionPeer.java:293)
  sql::StatementPeer.prepare (StatementPeer.java:39)
  sql::Statement.prepare (Statement.fan)
  migrations::Manager.logMigration (Manager.fan:143)
  migrations::Manager.prepare (Manager.fan:94)
  migrations::ManagerWithPostgreSqlTest.testMigrateToOne (ManagerWithPostgreSqlTest.fan:153)
  migrations::ManagerWithPostgreSqlTest.testMigrateToTwo (ManagerWithPostgreSqlTest.fan:190)
  migrations::ManagerWithPostgreSqlTest.testMigrateToThree (ManagerWithPostgreSqlTest.fan:251)
  migrations::ManagerWithPostgreSqlTest.testMigrateFromThree (ManagerWithPostgreSqlTest.fan:309)
  migrations::ManagerWithPostgreSqlTest.testMigrateFromTwo (ManagerWithPostgreSqlTest.fan:340)
  java.lang.reflect.Method.invoke (Method.java:597)
  fan.sys.Method.invoke (Method.java:536)
  fan.sys.Method$MethodFunc.callList (Method.java:188)
  fan.sys.Method.callList (Method.java:147)
  fanx.tools.Fant.runTest (Fant.java:174)
  fanx.tools.Fant.test (Fant.java:94)
  fanx.tools.Fant.test (Fant.java:30)
  fanx.tools.Fant.run (Fant.java:259)
  fanx.tools.Fant.main (Fant.java:295)
Cause:
  sys::Err: org.postgresql.util.PSQLException: Returning autogenerated keys is not supported.
    org.postgresql.jdbc3.AbstractJdbc3Connection.prepareStatement (AbstractJdbc3Connection.java:352)
    sql::StatementPeer.prepare (StatementPeer.java:34)
    sql::Statement.prepare (Statement.fan)
    migrations::Manager.logMigration (Manager.fan:143)
    migrations::Manager.prepare (Manager.fan:94)
    migrations::ManagerWithPostgreSqlTest.testMigrateToOne (ManagerWithPostgreSqlTest.fan:153)
    migrations::ManagerWithPostgreSqlTest.testMigrateToTwo (ManagerWithPostgreSqlTest.fan:190)
    migrations::ManagerWithPostgreSqlTest.testMigrateToThree (ManagerWithPostgreSqlTest.fan:251)
    migrations::ManagerWithPostgreSqlTest.testMigrateFromThree (ManagerWithPostgreSqlTest.fan:309)
    migrations::ManagerWithPostgreSqlTest.testMigrateFromTwo (ManagerWithPostgreSqlTest.fan:340)
    java.lang.reflect.Method.invoke (Method.java:597)
    fan.sys.Method.invoke (Method.java:536)
    fan.sys.Method$MethodFunc.callList (Method.java:188)
    fan.sys.Method.callList (Method.java:147)
    fanx.tools.Fant.runTest (Fant.java:174)
    fanx.tools.Fant.test (Fant.java:94)
    fanx.tools.Fant.test (Fant.java:30)
    fanx.tools.Fant.run (Fant.java:259)
    fanx.tools.Fant.main (Fant.java:295)

Originating at:

try
{
  stmt = self.conn.peer.jconn.prepareStatement(translated, java.sql.Statement.RETURN_GENERATED_KEYS);
  prepared = true;
}
catch (SQLException ex)
{
  throw ConnectionPeer.err(ex);
}

This is occurring running against PostgreSql 8.3 running on OS X (via macports) and connecting with the JDBC driver postgresql-8.3-605.jdbc4.jar.

Having google'd around a bit, the common solution seems to be "don't do it". I'm not sure where the check best goes, but it seems that there needs to be something like:

stmt = db.equals("postgres")
           ? self.conn.peer.jconn.prepareStatement(translated)
           : self.conn.peer.jconn.prepareStatement(translated, java.sql.Statement.RETURN_GENERATED_KEYS);

brian Fri 19 Feb 2010

Sorry about that - missed that spot.

Can you try out this changeset

msl Sat 20 Feb 2010

Thanks Brian - works perfectly now. Postgres support for migrations is also now in place.

go4 Sat 25 Sep 2010

I'am trying the postgreSql,still catch this error sys::Err: org.postgresql.util.PSQLException: ERROR: syntax error at or near "RETURNING".

The postgressql driver can't deal with autoGenerateKeys correctly.

I introduce an explicit returnAutoGenerateKeys parameter to solve: execute([Str:Obj]? params := null,Bool returnAutoGenerateKeys:=true)

another: Most of the time,I know the index of columns. What about this convenience api: row.get(row.cols[0]) => row.getByIndex(0)

brian Mon 27 Sep 2010

postgres is throwing an exception because of the auto-generated keys?

if you look at StatementPeer.java you can see how it works, but unless the JDBC driver explicitly declares that auto-generated keys are supported, then it doesn't try to use auto-gen keys

can you explain the problem in a bit more detail with a stack trace?

go4 Tue 28 Sep 2010

The auto-generated keys are supported by PostgreSQL. The insert statement always return the first field ,whatever it's a generated primary key. The create table and drop table statement throw this error:

sql::SqlErr: ERROR: syntax error at or near "RETURNING"
  :16
  sql::ConnectionPeer.err (ConnectionPeer.java:293)
  sql::StatementPeer.execute (StatementPeer.java:226)
  sql::Statement.execute (Statement.fan)
  sql::Statement.execute (Statement.fan)
  myfan::MySql.test (MySql.fan:52)
  myfan::MySql.main (MySql.fan:22)
  java.lang.reflect.Method.invoke (Unknown)
  fan.sys.Method.invoke (Method.java:536)
  fan.sys.Method$MethodFunc.callList (Method.java:182)
  fan.sys.Method.callList (Method.java:147)
  fanx.tools.Fan.callMain (Fan.java:135)
  fanx.tools.Fan.executeType (Fan.java:102)
  fanx.tools.Fan.execute (Fan.java:38)
  fanx.tools.Fan.run (Fan.java:236)
  fanx.tools.Fan.main (Fan.java:274)
Cause:
  sys::Err: org.postgresql.util.PSQLException: ERROR: syntax error at or near "RETURNING"
  :16
    org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse (QueryExecutorImpl.java:2102)
    org.postgresql.core.v3.QueryExecutorImpl.processResults (QueryExecutorImpl.java:1835)
    org.postgresql.core.v3.QueryExecutorImpl.execute (QueryExecutorImpl.java:257)
    org.postgresql.jdbc2.AbstractJdbc2Statement.execute (AbstractJdbc2Statement.java:500)
    org.postgresql.jdbc2.AbstractJdbc2Statement.executeWithFlags (AbstractJdbc2Statement.java:374)
    org.postgresql.jdbc2.AbstractJdbc2Statement.executeUpdate (AbstractJdbc2Statement.java:302)
    org.postgresql.jdbc3.AbstractJdbc3Statement.executeUpdate (AbstractJdbc3Statement.java:147)
    sql::StatementPeer.execute (StatementPeer.java:215)
    sql::Statement.execute (Statement.fan)
    sql::Statement.execute (Statement.fan)
    myfan::MySql.test (MySql.fan:52)
    myfan::MySql.main (MySql.fan:22)
    java.lang.reflect.Method.invoke (Unknown)
    fan.sys.Method.invoke (Method.java:536)
    fan.sys.Method$MethodFunc.callList (Method.java:182)
    fan.sys.Method.callList (Method.java:147)
    fanx.tools.Fan.callMain (Fan.java:135)
    fanx.tools.Fan.executeType (Fan.java:102)
    fanx.tools.Fan.execute (Fan.java:38)
    fanx.tools.Fan.run (Fan.java:236)
    2 More...

Maybe the postgresql driver do not ignore the RETURN_GENERATED_KEYS parameter,and try to return something. I'am using PostgreSQL 8.4.1,and postgresql-8.4-701.jdbc4.jar/postgresql-9.0-801.jdbc4.jar.

Thank you.

brian Tue 28 Sep 2010

What does your SQL look like? A simple insert?

go4 Tue 28 Sep 2010

This error is obvious

r:=db.sql("insert into obj(age)values(123)").execute()
echo(r)	//[123]
db.sql("drop table obj").execute	//sql::SqlErr: ERROR: syntax error at or near "RETURNING"
db.sql("create table obj(age int,pos bigserial)").execute

go4 Tue 28 Sep 2010

I modified the sources code , like this:

StatementPeer:
public Object execute(Statement self, Map params,boolean returnAutoGenerate)
{
  try
  {
    if (prepared)
    {
      setParameters(params);
    }
    else
    {
      createStatement(self);
    }
    try{
      if(returnAutoGenerate){
        int rows = stmt.executeUpdate(self.sql, autoGenKeyMode(self));
        return executeResult(self, rows);
      }else{
        return stmt.executeUpdate(self.sql);
      }
    }
    finally
    {
      stmt.close();
    }
  }
  catch (SQLException ex)
  {
    throw ConnectionPeer.err(ex);
  }

}

This code runs well

brian Tue 28 Sep 2010

So the insert works correctly?

And the drop throws an exception?

go4 Wed 29 Sep 2010

insert returning first column, but not prime key. drop/create throws an exception directly.

brian Wed 29 Sep 2010

@go4,

What it sounds like to me is that postgres reports that it does support auto-generated. And it does support it for "insert" updates. But does not handle using the auto-generated key flag for non-insert updates. If this is the case, then I would say the Postgres JDBC driver has a bug because the Javadoc spec is pretty clear that the driver should ignore this flags on non-insert.

But anyways we need to figure out a clean workaround without cluttering up the API. Can you try this patch against your setup and let me know if it resolves the problem?

diff --git a/src/sql/java/StatementPeer.java b/src/sql/java/StatementPeer.java
--- a/src/sql/java/StatementPeer.java
+++ b/src/sql/java/StatementPeer.java
@@ -229,7 +229,7 @@
 
   static int autoGenKeyMode(Statement self)
   {
-    return self.conn.peer.supportsGetGenKeys ?
+    return self.conn.peer.supportsGetGenKeys && FanStr.indexIgnoreCase(self.sql, "insert") != null ?
            java.sql.Statement.RETURN_GENERATED_KEYS :
            java.sql.Statement.NO_GENERATED_KEYS;
   }

go4 Thu 30 Sep 2010

Thank you.I will try it later.

go4 Tue 5 Oct 2010

Hi,brian.

There are still two errors.

1.StatementPeer.java line 245:

List keys = new List(Sys.IntType);
while (rs.next()) keys.add(rs.getLong(1));
if (!keys.isEmpty()) return keys;

rs.getLong(1),but my pk is String.The postgreSQL return a this pk,although it is not autoGenerate

2.StatementPeer.java line 478:

boolean valid =
   ((ch >= 'a') && (ch <= 'z')) ||
   ((ch >= 'A') && (ch <= 'Z')) ||
   ((ch >= '0') && (ch <= '9'));

in fact _ is valid character.SQL is not case-sensitive,'_' is often used.

brian Tue 5 Oct 2010

@go4

  1. Can you confirm that the patch listed above works for you on Postgres?
  2. Are you saying that your auto-generated keys are Strings, not ints? I am not sure I understand the problem.
  3. I will fix the "_" issue

go4 Tue 5 Oct 2010

The postgreSql always return the new created row,no matter whether having an autoGeneratedKey or not.

Sorry,my English is very poor,I can't tell clearly.

brian Tue 5 Oct 2010

@go4 - why don't you email me directly to resolve your various issues

Login or Signup to reply.