Skip site navigation (1) Skip section navigation (2)

Re: Patch for Statement.getGeneratedKeys()

From: Ken Johanson <pg-user(at)kensystem(dot)com>
To: pgsql-jdbc(at)postgresql(dot)org
Subject: Re: Patch for Statement.getGeneratedKeys()
Date: 2008-01-08 05:16:55
Message-ID: 47830747.5060707@kensystem.com (view raw or flat)
Thread:
Lists: pgsql-generalpgsql-jdbc
Kris,
Incomplete / short response below:
> 
> 
> Here it claims to have lines 159 to 172, but it only has 10 lines of 
> text. Perhaps you need a Netbeans upgrade or you need to use some other 
> CVS client.

I am upgrading my NB now and we'll see how the next one comes out..

> 
> Reading through the patch I have the following comments:
> 
> 1) Why does executeUpdateGetResults have support for isFunction?  
> Doesn't that require preparedQuery != null?  Even if not, shouldn't the 
> replaceProcessing be before the isFunction check?

I'm going to let you suggest code for this method, as I don't even 
recall coding it (a long time ago perhaps and lazy copy-paste). I do not 
know the driver innards enough to answer this (I am a relative novice 
even at the formal JDBC API level).

> 
> 2) Shouldn't the result for a generated key result be stored in some 
> place more specific?  Right now can't you issue executeQuery() and then 
> call getGeneratedKeys()?

Again I am not familiar with all the use cases but presume you mean, 
allowing for calling a query (non-DML?) and then expect the keys to be 
available?..

> 
> 3) Generated keys should work for more than just insert, at least in 
> postgres' design.  RETURNING works for all of INSERT/UPDATE/DELETE.
> 

Will add these unless you suggest code first.

> 4) As discussed previously on -general the code in executeUpdate(String, 
> int[]) shouldn't be using information_schema because that has additional 
> permission requirements.  Also it looks like it's got sql injection 
> problems.

I am waiting on this and #9.b; for an answer to a prior question about: 
is it is possible to determine the connection's db and schema names to 
normalize those array elements (in the case where just the 
[schema]tablename is provided). Is it possible without a round trip to 
know what these should be?

> 
> 5) There's no need to check connection instanceof 
> AbstractJdbc2Connection in executeUpdate(String, String[]).  That will 
> always be true.

OK

> 
> 6) There's no need to split this up for translation purposes, just make 
> it one string:
> 
>     throw new PSQLException(GT.tr("Server version does not support 
> returning generated keys.")+" (< "+"8.2"+")", PSQLState.NOT_IMPLEMENTED);
> 

Unless the code will not work I will elect to keep support for 
translation should someone want to enter these.

> 7) executeUpdate(String, String[]) does not correctly escape the 
> columnNames provided if the values have embedded quotes.
> 

Thank you, I will apply appendEscapedIdentifier().

> 8) Utils.needsQuoted is unused and should be removed.

OK

> 
> 9) Utils.getInsertIds doesn't look right.  Looks like it will return 
> "into" for something like "insert       into xxx (...)".  It doesn't 
> look like it will work for names with quotes in them like "x""y". 

It requires a context-sensitive (int)start argument, to position the 
search start onto the first keyword, i.e INTO for the case of INSERTs. I 
tested quoted quote-less Ids. As stated earlier it is incomplete pending 
a way to pass-in the current DB & schema names.


9.b):
  Also
> the requirement that a query uses a completely qualified name 
> database.schema.table is quite onerous.  Additionally the fact that this 
> requirement is not checked will result in many 
> ArrayIndexOutOfBoundsExceptions.

(see above). I believe that a fully qualified value may be needed in the 
  case where the table is fully qualified but (for some reason) not in 
the same context as the RETURNING... this can happen, yes? Or does 
RETURNING always refer to the acted-on table?

> 
> 10) What's the purpose of Utils.position?  How is this better than 
> String.indexOf with lowercase strings?

It save a potential buffer allocation should String.class find a 
case-fold is necessary (esp in large query).

query.toLowerCase().indexOf(s);

Utils.position is not strictly needed and I will remove it if you prefer.

Ken



In response to

Responses

pgsql-jdbc by date

Next:From: Kris JurkaDate: 2008-01-08 06:49:35
Subject: Re: Missing fields in getColumns() result
Previous:From: Gregory StarkDate: 2008-01-07 20:08:48
Subject: Re: TypeInfoCache

pgsql-general by date

Next:From: Ow Mun HengDate: 2008-01-08 05:34:49
Subject: Re: Announcing PostgreSQL RPM Buildfarm
Previous:From: Rajaram JDate: 2008-01-08 05:11:48
Subject: postgres dump cores

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group