Re: Patch for Statement.getGeneratedKeys()

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